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

Add image-pull-secret option #54

Merged
merged 14 commits into from
Feb 9, 2023
Merged

Conversation

clbx
Copy link
Contributor

@clbx clbx commented Jan 5, 2023

Added image-pull-secret option to specify an image pull secret for a job that uses an image in a private registry.

@clbx clbx requested review from tgolsson and keith as code owners January 5, 2023 21:21
@tgolsson
Copy link
Member

Hey @clbx!

Thanks for the PR. I'm back from Christmas vacations today so will try to get to this during the week.

@tgolsson tgolsson self-assigned this Jan 10, 2023
@tgolsson
Copy link
Member

Hey @clbx!

Sorry for not getting back to you earlier. I had a quick look and I think it looks good. I'm just wondering if you can elaborate (maybe here and/or in the docs) on why this would preferable over e.g. configuring automatic secrets with the service account. More fine-grained permissions?

@clbx
Copy link
Contributor Author

clbx commented Feb 8, 2023

Honestly, I didn't know that assigning a pull secret to a service account was a feature.

Using a Service Account covers our use case, but I think there could be use cases for having it independent of service accounts as it's a relatively common design pattern to specify image pull secrets without defining them in a service account.

lib/job.jsonnet Outdated
@@ -44,6 +45,7 @@ function(jobName, agentEnv={}, stepEnvFile='', patchFunc=identity) patchFunc({
BUILDKITE_PLUGIN_K8S_WORKDIR: std.join('/', [env.BUILDKITE_BUILD_PATH, buildSubPath]),
BUILDKITE_PLUGIN_K8S_JOB_TTL_SECONDS_AFTER_FINISHED: '86400',
BUILDKITE_PLUGIN_K8S_JOB_BACKOFF_LIMIT: '0',
BUILDKITE_BUILD_CREATOR_TEAMS: '',
Copy link
Member

Choose a reason for hiding this comment

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

This seems unrelated! I guess this is related to #59?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. If you’re not part of a team, the plugins crashes. If I remember correctly teams are a paid feature, so this fixes the plug-in for anyone on the free tier.

Copy link
Member

Choose a reason for hiding this comment

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

Right! Just for cleanliness; can you remove it from this PR? I've fixed this another way while debugging a bunch of other crashes in #60, and I'd like to keep each PR clean :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@tgolsson tgolsson 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 for the contribution. I'll see if I can squeeze out a release next week.

@tgolsson tgolsson merged commit 9a156ca into EmbarkStudios:master Feb 9, 2023
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