Skip to content

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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

droot
Copy link
Contributor

@droot droot commented Sep 16, 2022

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

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

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants