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

feat(k8s): custom annotations for builder pods #3365

Merged
merged 8 commits into from
Nov 23, 2022
Merged

Conversation

stefreak
Copy link
Member

@stefreak stefreak commented Nov 22, 2022

What this PR does / why we need it:
Allow specifying custom annotations for garden-
managed pods when using in-cluster building with
buildkit or kaniko.

This is useful when users want to alter how certain
components, like the cluster-autoscaler treat these
pods.

Which issue(s) this PR fixes:

Fixes #2628

Special notes for your reviewer:

Allow specifying custom annotations for garden-
managed pods when using in-cluster building with
buildkit or kaniko.

This is useful when users want to alter how certain
components, like the cluster-autoscaler treat these
pods.

Fixes #2628
@stefreak stefreak marked this pull request as ready for review November 22, 2022 13:48
@stefreak stefreak requested a review from Orzelius November 22, 2022 13:48
@stefreak
Copy link
Member Author

@Orzelius I have no clue why the circleci tests did not run on this PR 🤔

it("should return a Kubernetes Deployment manifest for buildkit in-cluster-builder", () => {
const result = getBuildkitDeployment(provider, "authSecretName", [{ name: "imagePullSecretName" }])
expect(result.kind).eql("Deployment")
expect(result.metadata).eql({
Copy link
Member Author

Choose a reason for hiding this comment

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

I split this into separate expect()s because otherwise the diffs would be useless because of https://stackoverflow.com/questions/70962366/with-mocha-chai-how-can-i-make-it-show-lines-skipped

@stefreak stefreak force-pushed the feat-builder-annotations branch from c0b9d14 to 51b5b18 Compare November 22, 2022 14:05
@stefreak stefreak force-pushed the feat-builder-annotations branch from 51b5b18 to f6cab9e Compare November 22, 2022 14:13
Copy link
Contributor

@Orzelius Orzelius left a comment

Choose a reason for hiding this comment

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

Left a few comments. Good job on the tests!

util: joi.object().keys({
tolerations: joiSparseArray(tolerationSchema()).description(
"Specify tolerations to apply to each garden-util Pod."
"Specify tolerations to apply to the garden-util Pod."
Copy link
Contributor

@Orzelius Orzelius Nov 22, 2022

Choose a reason for hiding this comment

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

I think should stay as "each" or even maybe

Suggested change
"Specify tolerations to apply to the garden-util Pod."
"Specify tolerations to apply to garden-util pods."

There's a garden-util pod per garden managed kubernetes namespace so its technically not a singular "the pod"

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, per namespace there is only one; if I run garden deploy it will only change it in one namespace, right? That's why I preferred the signular.

But I am ok with changing it to the plural, changed it in the latest commit 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

if I run garden deploy it will only change it in one namespace, right?

yes, but if you change the project config it will change for everyone resulting in changes in all namespaces

@stefreak stefreak requested a review from Orzelius November 22, 2022 14:58
Orzelius
Orzelius previously approved these changes Nov 22, 2022
When passing manifests with undefined values to
compareDeployedResources,
it would that the to-be-deployed manifest is not a superset of
the deployed manifest, even though undefined values are treated
the same as non-existing values.

This commit fixes that bug.
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.

[FEATURE]: allow setting annotations on garden managed pods
2 participants