Skip to content

chore: CCIE-3986 use persist-credentials: false in checkout #343

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wontonst
Copy link
Contributor

@wontonst wontonst commented Feb 3, 2025

I did a manual inspection of the files but no guarantees that this won't break something.

If anyone knows of a place where we run privileged git commands other than update manifest, please let me know.

If this PR doesn't end up breaking anything I'll open PRs for other repos using the same approach.

@@ -51,6 +51,7 @@ runs:
}
- uses: actions/checkout@v4
with:
persist-credentials: true # creds used in Update Argus Manifest step
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if we just accept this risk or if we want to do something about this

in my experience, automation doesn't make commits against the repo. the name of the artifact is also not something baked into the repo that get constantly updated. it's usually an external system managing the mapping of git hash to docker img tag, and that system will also keep track of what's deployed in prd/staging. sounds like that should be all done in argus so I'm curious if that's something that's possible/something we've considered doing.

@wontonst wontonst marked this pull request as ready for review February 4, 2025 22:52
@wontonst wontonst requested a review from a team February 4, 2025 22:52
Copy link
Contributor

@jakeyheath jakeyheath left a comment

Choose a reason for hiding this comment

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

I'll trust you if you test it out and everything works. I'm not exactly familiar with this feature, but would like to see more familiarity with this pattern if it does work.

@wontonst
Copy link
Contributor Author

I'll trust you if you test it out and everything works. I'm not exactly familiar with this feature, but would like to see more familiarity with this pattern if it does work.

it's hard to test this out since this affects consumers of these actions. eg if someone is doing a git push using the creds obtained in one of the actions affected by this change, that command will break.

it might make sense to break their workflow anyway and have them use GITHUB_TOKEN which lets them decide whether they want to grant write permissions

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants