[design proposal] Add config option 'deploy.config.transformableAllowList' #6236
Conversation
Codecov Report
@@ Coverage Diff @@
## main #6236 +/- ##
=======================================
Coverage 70.80% 70.80%
=======================================
Files 494 494
Lines 22327 22327
=======================================
Hits 15808 15808
Misses 5499 5499
Partials 1020 1020 Continue to review full report at Codecov.
|
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
shawnzhu
changed the title
|
@tejal29 would you please review again? |
|
We missed covering this in Wed's meeting. I will update the DD soon. Thanks! |
| - Group: example.com | ||
| Kind: Application |
There was a problem hiding this comment.
It would be nice if this could be more compact. And we should also be able to specify versions here too as they may have dramatically different shapes. In Kubernetes, these objects are referred to as resource types, and they have versions.
Then I think we should allow some configurability to determine whether (and where) we can perform certain transforms. Currently Skaffold:
- applies labels: Skaffold looks for any field named
metadataand add alabelsblock if missing. This action breaks CRDs, which is why we blocked this behind an allowlist. - rewrite images: Skaffold looks for any field named
image
So I suggest something like the following for the simple cases (rewrite everything based on field names):
- type: pod # no group, implicitly all versions
- type: batch/Job # group, implicitly all versions
- type: extensions/v1beta1/Deployment # specific version
And we might try something like adding images and labels fields that provide a list of JSON-path-like paths. Maybe with * support?
- type: openfaas.com/v1/Function
images: [spec.image]
labels: [spec.metadata.labels, spec.labels] # https://www.openfaas.com/blog/manage-functions-with-kubectl/
- type: apps/v1/Deployment
images: [spec.template.spec.initContainers.*.image, spec.template.spec.containers.*.image]
labels: [spec.metadata.labels, spec.template.metadata.labels]
Nit: these fields should be lower-cased in keeping with the rest (k8s yaml uses lower case)
There was a problem hiding this comment.
I like such compact configuration and the explict specification for images and labels.
How about this:
- adopt the compact configuration
- implement applies labels so it only rewrite allowed labels of resource types in allowlist
- only rewrite image of resource types in allowlist
Any guidance on the implementation like which module to change would also be appreicated!
|
@shawnzhu Sorry for the delay! LGTM! Do you want to start working on it? |
|
@tejal29 Thanks! and yes, let me try. |
|
Hi @shawnzhu — do you have the bandwidth to continue on with this effort? Can I help? |
|
@briandealwis I'm glad you offer help. I was learning how to parse 3 different expressions of resource types with k8s API, but haven't figure out a clue yet. if you can provide pointers or start work on one expression of resource type, that would be awesome. |
Supports: #4081
Related: Relevant tracking issues, for context
Merge before/after: Dependent or prerequisite PRs
Description
This is important for working with CRDs that needs transform by skaffold.
User facing changes (remove if N/A)
It introduces new schema change so user may need to update the
skaffold.yamlto pick up this new feature