Replacements should support an inline value as a source

grozan commented 9 months ago

Eschewed features

[X] 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! 🎉

koba1t commented 8 months ago

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

/triage accepted /help

k8s-ci-robot commented 8 months ago

@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](https://github.com/kubernetes-sigs/kustomize/issues/5516): >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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

cunyat commented 7 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

Hi @varshaprasad96 Could you check this feature request?

kubernetes-sigs / kustomize