Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: updated config file permission requirements for windows (#9666) #9732

Merged
merged 4 commits into from Jun 21, 2022

Conversation

gdsoumya
Copy link
Member

@gdsoumya gdsoumya commented Jun 21, 2022

Fixes #9666
Fixes #9702

This PR updates the permission requirements for windows to 0444 or 0666 instead of 0400 and 0600. The config file produced by argocd cli on windows always appears to be having 0666 permission this is probably due to some limitations on windows acl, hence this PR updates the requirement specifically for windows for 0666 or 0444 permission instead of 0400/0600.

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).

Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com>
Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com>
@codecov
Copy link

codecov bot commented Jun 21, 2022

Codecov Report

Merging #9732 (a02f2fa) into master (a0ec77e) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #9732   +/-   ##
=======================================
  Coverage   45.85%   45.85%           
=======================================
  Files         226      227    +1     
  Lines       26795    26795           
=======================================
  Hits        12286    12286           
  Misses      12840    12840           
  Partials     1669     1669           
Impacted Files Coverage Δ
util/localconfig/localconfig.go 2.67% <ø> (-3.36%) ⬇️
util/localconfig/file_perm_unix.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0ec77e...a02f2fa. Read the comment docs.

Copy link
Collaborator

@alexmt alexmt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@@ -1,3 +1,5 @@
//go:build !windows
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have an equivalent test for windows?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From Slack:

I actually did try to create windows only tests but I just couldn't manage to set the permission properly with golang it just won't set it to something other than 0444 or 0666

lgtm. :-)

@crenshaw-dev crenshaw-dev enabled auto-merge (squash) June 21, 2022 18:35
@alexmt alexmt added the cherry-pick/2.4 Candidate for cherry picking into the 2.4 release branch label Jun 21, 2022
@alexmt
Copy link
Collaborator

alexmt commented Jun 21, 2022

PR fixes v2.4 regression. Adding cherry-pick label.

@crenshaw-dev crenshaw-dev changed the title fix: updated config file permission requirements for windows fix: updated config file permission requirements for windows (#9666) Jun 21, 2022
@crenshaw-dev crenshaw-dev merged commit 8870850 into argoproj:master Jun 21, 2022
crenshaw-dev pushed a commit that referenced this pull request Jun 21, 2022
* fix: reduced config file permission restriction on windows

Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com>

* fix: updated localconfig tests to check error

Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com>
@crenshaw-dev
Copy link
Collaborator

Cherry-picked onto release-2.4 for release with 2.4.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick/2.4 Candidate for cherry picking into the 2.4 release branch
Projects
None yet
3 participants