-
Notifications
You must be signed in to change notification settings - Fork 282
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
always use a fresh docker config for each step #756
Conversation
This avoids potential race condition that: - Allow steps to use credential from other steps - Causes steps to use incorrect credentials, if they are over-written by another step
This improves job isolation, preventing jobs from relying on authentication configured in other jobs. This is based on PR #678, but we've removed the stack parameter because the v5.0.0 release is pending and we're comfortable making a small breaking change.
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.
Straightforward change to improve step isolation. We've discussed this and decided it's better to make this (possibly) breaking change now as part of v5 release, and to bugfix it later if necessary. Original PR (#678) says this has been working well for them for quite some time.
Couldn't we do this in the |
@lox are you suggesting this doesn't need to be baked in for everyone? |
Setting it directly in the agent environment hook means we don't need to escape the eval. It also means we won't cat it out into build output with the rest of cfn-env. While I was there I also made the same change to windows.
This also fixes a shellcheck error SC2155 Instead of setting DOCKER_CONFIG directly, set BUILDKITE_DOCKER_CONFIG_TEMP_DIRECTORY and hydrate DOCKER_CONFIG for plugin steps. if this gets overriden by the user, they can manage their own DOCKER_CONFIG cleanup activities and we will always remove the temporary directory from the agent instance filesystem.
This improves job isolation, preventing jobs from relying on authentication configured in other jobs.
This is based on PR #678, but we've removed the stack parameter because the v5.0.0 release is pending and we're comfortable making a small breaking change.