8000 set-labels: added support for sourcing label values from other resources by droot · Pull Request #914 · kptdev/krm-functions-catalog · GitHub
[go: up one dir, main page]
Skip to content

set-labels: added support for sourcing label values from other resources#914

Open
droot wants to merge 2 commits intokptdev:mainfrom
droot:set-labels-source-value
Open

set-labels: added support for sourcing label values from other resources#914
droot wants to merge 2 commits intokptdev:mainfrom
droot:set-labels-source-value

Conversation

@droot
Copy link
Contributor
@droot droot commented Sep 16, 2022 •
edited
Loading

This PR adds support for specifying label values from fields in the other resources in the package.

An example setlabel configuration where we add myapp label and value comes from data.name field in package-context.yaml.

Note the labelsFrom. This change is also backward compatible since labelsFrom is new field.

# fn-config-setlabels.yaml
apiVersion: fn.kpt.dev/v1alpha1
kind: SetLabels
metadata: # kpt-merge: /set-labels
  name: set-labels
  labels:
    app: frontend
  annotations:
    config.kubernetes.io/local-config: "true"
    internal.kpt.dev/upstream-identifier: fn.kpt.dev|SetLabels|default|set-labels
labels:
  app: frontend
labelsFrom:
  - label: myapp
    source:
      kind: ConfigMap
      name: kptfile.kpt.dev
      fieldPath: data.name

/cc @yuwenma @natasha41575

Note: I tested this manually. Will add tests if y'all agree with the direction.

@yuwenma
Copy link
Contributor
yuwenma commented Sep 16, 2022
8000

I plan to add a generic dynamic input support in the go SDK.
Adding it directly to set-labels will introduce several problems.
Currently I'd suggest use apply-replacement and set-labels together to achieve the same goal, and do not use a new approach.

@natasha41575
Copy link
Contributor
natasha41575 commented Sep 16, 2022

Currently I'd suggest use apply-replacement and set-labels together to achieve the same goal, and do not use a new approach.

I was wondering the same thing. Is it too much to use apply-replacements to target this SetLabels resource to get the config you need?

@yuwenma
Copy link
Contributor
yuwenma commented Sep 16, 2022

Currently I'd suggest use apply-replacement and set-labels together to achieve the same goal, and do not use a new approach.

I was wondering the same thing. Is it too much to use apply-replacements to target this SetLabels resource to get the config you need?

It is. But the set-labels function is a curated function and we already have plans to change its functionConfig( yuanyi's original proposal). Adding a new feature ( i.e. the "labelsFrom" syntax) which we will have a more generic solution for (the go SDK) does not seem worth the user migration burden and the function owner's maintenance efforts.

@yuwenma
Copy link
Contributor
yuwenma commented Sep 16, 2022

@droot Is this a critical feature for Bank-of-Anthos? If so, I can prioritize this work and do it on the go-SDK side. Otherwise, I'd recommend we use apply-replacements and set-labels.

@wleese
Copy link
Contributor
wleese commented Sep 30, 2022

Would love to see this go in - currently using a workaround to push values into the set-labels function config

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mengqiy mengqiy Awaiting requested review from mengqiy

@sdowell sdowell Awaiting requested review from sdowell

@yuwenma yuwenma Awaiting requested review from yuwenma

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@droot @yuwenma @natasha41575 @wleese
0