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

Replacements should support an inline value as a source #5516

Open
1 of 2 tasks
grozan opened this issue Jan 22, 2024 · 8 comments
Open
1 of 2 tasks

Replacements should support an inline value as a source #5516

grozan opened this issue Jan 22, 2024 · 8 comments
Assignees
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@grozan
Copy link

grozan commented Jan 22, 2024

Eschewed features

  • This issue is not requesting templating, unstuctured edits, build-time side-effects from args or env vars, or any other eschewed feature.

What would you like to have added?

As suggested in other issues, like here, it would be very nice to be able to specify a value directly, inline.
The target value is not always a field of another object. It can simply be a value that the user knows/wants.

Why is this needed?

There is the workaround of declaring a new object like a configmap and annotating it with config.kubernetes.io/local-config: "true" but this adds extra complexity and bloat to the definition of the desired state IMO

A simple replacement should look something like

replacements:
- source:
    value: bar
  targets:
  - select:
      name: hello
      kind: Job
    fieldPaths:
    - spec.template.spec.containers.[name=hello].env.[name=foo].value

without any kind of place holder object to define

Can you accomplish the motivating task without this feature, and if so, how?

yes, with a "fake" object as mentioned, but it's not an elegant solution at all

What other solutions have you considered?

json patches, but they don't support defining list items by their name ("/spec/template/spec/containers/[name=hello]/env/[name=foo].value" is NOT possible) as mentioned in other issues like #4950

Anything else we should know?

No response

Feature ownership

  • I am interested in contributing this feature myself! 🎉
@grozan grozan added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 22, 2024
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jan 22, 2024
@koba1t
Copy link
Member

koba1t commented Feb 14, 2024

I think it's an interesting feature and that helps write their configuration without temporary configMap.

/triage accepted
/help

@k8s-ci-robot
Copy link
Contributor

@koba1t:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

  • Why are we solving this issue?
  • To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
  • Does this issue have zero to low barrier of entry?
  • How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

I think it's an interesting feature and that helps write their configuration without temporary configMap.

/triage accepted
/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 14, 2024
@cunyat
Copy link

cunyat commented Mar 9, 2024

Hi there! Currently working on it and got a working solution for the given example.

Anyway, I have some doubts:

  • The value should accept only scalar values or should it allow lists or objects?
  • I'm assuming that the value field must be given alone, no other selection fields or fieldPath can be present.
  • I suppose that docs must be updated, only this file?

In my head makes more sense to add an extra key to the replacement object:

replacements:
- value: bar
  targets:
  - select:
      name: hello
      kind: Job
    fieldPaths:
    - spec.template.spec.containers.[name=hello].env.[name=foo].value

/assign

@koba1t
Copy link
Member

koba1t commented Mar 28, 2024

I'm so sorry for the delayed response.
I appreciate your contribution!

The value should accept only scalar values or should it allow lists or objects?

I believe that only one scalar value is accepted from the source due to current replacement transformers, and I believe we need to keep that.

I'm assuming that the value field must be given alone, no other selection fields or fieldPath can be present.

I think it is better to define the new value than select the source scalar value, and we do not accept selecting both fields.
I believe that the new name is better for containing more explanation, like the sourceValue field.

/cc @varshaprasad96
This seems to mean adding a new interface for the existing function. What do you think?
our current rule

I suppose that docs must be updated, only this file?

Currently, replacements docs are here is another one.

@grozan
Copy link
Author

grozan commented Mar 28, 2024

Hi,
thinking out loud here, but couldn't this also be an opportunity to introduce an easy-to-use mechanism to delete list items ?

jsonpatch does not support paths like spec.template.spec.containers.[name=hello].env.[name=foo] for example, unfortunately, so we have to rely on indexes to target a specific entry in a list, which is neither robust to changes, nor self-documenting.

could we imagine a special sourceValue value like null, or -, or something else, to mean "replace it by nothing, aka delete the entry"?

I mean

replacements:
- sourceValue: bar
  targets:
  - select:
      name: hello
      kind: Job
    fieldPaths:
    - spec.template.spec.containers.[name=hello].env.[name=foo].value

would set the value of the foo ENV variable of the hello container to bar

replacements:
- sourceValue: null
  targets:
  - select:
      name: hello
      kind: Job
    fieldPaths:
    - spec.template.spec.containers.[name=hello].env.[name=foo]

would remove the foo ENV variable from the hello container. It's completely gone from the env list

cunyat added a commit to cunyat/kustomize that referenced this issue Mar 28, 2024
Introduces a new key on replacement `sourceValue` for replacing from a
static value instead of sourcing from another object.

Solves kubernetes-sigs#5516
cunyat added a commit to cunyat/kustomize that referenced this issue Mar 28, 2024
Introduces `sourceValue` on replacement object for replacing from a
static value instead of sourcing from another object.

Solves kubernetes-sigs#5516
@cunyat
Copy link

cunyat commented Jun 9, 2024

Hey @koba1t,
I issued a PR few months ago, but seems that is not getting reviewed :)
If there is something I can do to get it reviewed please tell me,

@koba1t
Copy link
Member

koba1t commented Jun 11, 2024

I issued a PR few months ago, but seems that is not getting reviewed :)

I think that it is better to get a review from another maintainer...
Because this tries to add a new function containing API change.

@koba1t
Copy link
Member

koba1t commented Jun 11, 2024

Hi @varshaprasad96
Could you check this feature request?

cunyat added a commit to cunyat/kustomize that referenced this issue Jun 12, 2024
Introduces `sourceValue` on replacement object for replacing from a
static value instead of sourcing from another object.

Solves kubernetes-sigs#5516
cunyat added a commit to cunyat/kustomize that referenced this issue Jul 25, 2024
Introduces `sourceValue` on replacement object for replacing from a
static value instead of sourcing from another object.

Solves kubernetes-sigs#5516
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

4 participants