kubernetes driver // allow custom annotations and labels #1938
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3e506e3 to
1996822
Compare
AkihiroSuda
reviewed
Jul 17, 2023
150ac01 to
1d9422b
Compare
AkihiroSuda
reviewed
Jul 17, 2023
AkihiroSuda
reviewed
Jul 17, 2023
1d9422b to
11f376c
Compare
AkihiroSuda
approved these changes
Jul 17, 2023
crazy-max
reviewed
Jul 19, 2023
docs/reference/buildx_create.md
Outdated
| @@ -141,7 +141,7 @@ to achieve that. | |||
|
|
|||
| Passes additional driver-specific options. | |||
|
|
|||
| Note: When using quoted values for example for the `nodeselector` or | |||
| Note: When using quoted values for example for the `nodeselector`, `annotations` or | |||
There was a problem hiding this comment.
Suggested change
| Note: When using quoted values for example for the `nodeselector`, `annotations` or | |
| Note: When using quoted values for example for the `nodeselector`, `annotations`, `labels` or |
There was a problem hiding this comment.
I was expecting this comment :)
Should I remove for example from the sentence, since it's supposed to be all-inclusive?
Suggested change
| Note: When using quoted values for example for the `nodeselector`, `annotations` or | |
| Note: When using quoted values for the `nodeselector`, `annotations`, `labels` or |
There was a problem hiding this comment.
(note: I applied my suggestion)
crazy-max
reviewed
Jul 19, 2023
crazy-max
reviewed
Jul 19, 2023
driver/kubernetes/factory_test.go
Outdated
Comment on lines
218
to
220
|
|
||
| _, _, _, err := f.processDriverOpts(cfg.Name, "test", cfg) | ||
|
|
There was a problem hiding this comment.
I applied this comment on the other identical surrounding tests as well. I also noticed an extra ns := "test" at the beginning that was not necessary since it was rewritten in the next call.
11f376c to
7a228d6
Compare
Co-authored-by: Akihiro Suda <suda.kyoto@gmail.com> Signed-off-by: Jonathan Piché <jpiche@coveo.com>
7a228d6 to
9c06f38
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This implements custom annotations and labels to be included in kubernetes manifests. Currently, the user has to patch them in a subsequent call.
Motivation: We provide a tool for developers to create/remove buildx builders on a shared k8s cluster. To prevent wasting resources, the buildx deployments are patched with a label (for filtering) and an annotation (an
expire-aftertimestamp). A cron janitor eventually finds and removes them. The user may also refresh the timestamp in order to retain their cache for longer if they desire.Issue: Because creating the builder and patching the annotations requires two calls, I need to handle the cases when the
createcall fails. This isn't always possible in an exception handler, for instance when this happens on a CI server that gets killed abruptly. This forces the janitor to include additional heuristics to find and remove the leaked deployments. It would be simpler if these annotations would be included from the start.