BUGFIX: pass full path to chown #70
Conversation
cert-manager-prow
bot
added
dco-signoff: yes
cert-manager-prow
bot
added
size/S
|
I wonder what we can do to get signal that this broke csi-driver sooner? Perhaps we can add an optional test job that fetches the csi-driver and attempts to build it with the PRs version of the csi-lib imported/required? i.e. |
Signed-off-by: Tim Ramlot <[email protected]>
cert-manager-prow
bot
added
size/XS
There is a lot we can do to improve this repository, including making the existing tests more real-life (eg. using a real file system). |
| for filename := range files { | ||
| // Set the uid to -1 which means don't change ownership in Go. | ||
| if err := os.Chown(filepath.Join(tsDirName, filename), -1, int(*fsGroup)); err != nil { | ||
| if err := os.Chown(filepath.Join(f.dataPathForVolumeID(meta.VolumeID), tsDirName, filename), -1, int(*fsGroup)); err != nil { |
There was a problem hiding this comment.
This is the minimal fix. This PR taught me about a lot about other possible improvements, they will be part of v0.9.0 instead.
This minimal fix matches what we were doing before: https://github.com/cert-manager/csi-lib/pull/65/files#diff-ba1745ccd4960836e72c7cf9c559b61ecceab32eb78f3be37d56d8c777c1d141L270
|
I'm happy to believe this is the minimal fix and it works, but it's not clear to me from this PR why this fix was required by updating the csi-driver deps. What actually changed to cause a break? Can we add that detail to this PR for future reference? |
I added the missing link to #65, which is where the bug was introduced (+ see the code link in the description for the exact line). |
SgtCoDFish
left a comment
SgtCoDFish
left a comment
There was a problem hiding this comment.
/lgtm
/approve
Thanks for confirming that it was upgrading csi-lib specifically which caused the issue! It seems to me this is worth merging as-is.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: SgtCoDFish The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cert-manager-prow
bot
added
the
approved
3 participants
Fixes bug introduced in #65.
Chown was using the
tsDirNameinstead of the full path: https://github.com/cert-manager/csi-lib/compare/main...inteon:csi-lib:bugfix?expand=1#diff-ba1745ccd4960836e72c7cf9c559b61ecceab32eb78f3be37d56d8c777c1d141L269This resulted in
WriteFilesreturning an error.This caused the dependency upgrade PR for csi-driver to fail: cert-manager/csi-driver#322
I'll add more improvements and extra tests in a follow-up PR.