-
Notifications
You must be signed in to change notification settings - Fork 453
Add "--password-file-reload" flag to refresh password during each sync cycle #976
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
base: master
Are you sure you want to change the base?
Add "--password-file-reload" flag to refresh password during each sync cycle #976
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vaibhavdesai137 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
|
Welcome @vaibhavdesai137! |
thockin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I need to make this happy is an e2e test (test_e2e.sh). Use a file in $WORK which we can change (write tmp file, rename).
The test could do something like:
- Run once with the correct password, verify the result.
- Change the password file contents to the wrong password
- Commit a change to the repo
- Verify the local sync did not change
- Change the password file back
- Verify it synced to the new change
Look at function e2e::auth_http_password_file() and function e2e::auth_askpass_url_sometimes_wrong() as examples?
| "the file from which the password or personal access token for git auth will be sourced") | ||
| flPasswordFileReload := pflag.Bool("password-file-reload", | ||
| envBool(false, "GITSYNC_PASSWORD_FILE_RELOAD"), | ||
| "reload the password from --password-file on each sync cycle (useful when password is rotated by an external system like Vault Dynamic Engine)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's not name specific products :)
| "reload the password from --password-file on each sync cycle (useful when password is rotated by an external system like Vault Dynamic Engine)") | |
| "reload the password from --password-file on each sync cycle, useful when password is rotated by an external system") |
| // These should all be mutually-exclusive configs. | ||
| for _, cred := range *flCredentials { | ||
| if err := git.StoreCredentials(ctx, cred.URL, cred.Username, cred.Password); err != nil { | ||
| password := cred.Password |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason NOT to just re-read the file on every loop?
IOW, why not get rid of the "Finish populating credentials" block above (L752) and reload it here, if cred.PasswordFile is set?
If you set the period to milliseconds it might hurt (but this is the wrong tool for that anyway). On the order of seconds or longer it seems irrelevant and net simpler.
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds flags for --hooks-async and --hooks-before-symlink
Which issue(s) this PR fixes:
Fixes #975
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
/kind feature