Revert documentation of Kustomize bug as a feature

[KnVerey]

This reverts commit b9da040 / PR #30348. That PR documented a bug in Kustomize as though it were a feature, and unfortunately nobody from the Kustomize team saw the PR.

The bug in question dates back to the origins of Kustomize, when some of the original code was copied out of kubectl, and this undesirable behaviour (for kustomize, not kubectl) went unnoticed in the adaptation. That explains why the code (written for kubectl) had comments that made it seem like the behaviour was intentional.

This behaviour is a clear violation of one of Kustomize's core principles, as documented in our Eschewed Features list:

Since this behaviour has been around unnoticed by us for so long, our plan is to to deprecate it and remove it with a major version bump rather than treating it like a bug fix: kubernetes-sigs/kustomize#4730. However, we would like to get the docs for it removed ASAP, as it is absolutely not something we want to encourage users to do.

cc @natasha41575

[natasha41575]

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 9fecd9756d6b3a6e207e374c957af4c749b50454

[netlify]

✅ Pull request preview available for checking

Built without sensitive environment variables

Name	Link
🔨 Latest commit	39fb094
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/62eac7b06899ee000854c0a9
😎 Deploy Preview	https://deploy-preview-35522--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[divya-mohan0209]

Thank you @KnVerey for this PR!

IMO instead of actually removing the whole section before deprecation/removal, it would be more appropriate to actually state that this is not a recommended approach & it will be deprecated/removed soon with an ETA. Maybe we could even add a caution admonition instead of a note. I know this is the way we typically treat features. However, given that there'll be a warning displayed if this method is followed in the future and there may be a few folks following this currently that might be a better approach. WDYT?

[sftim]

We should document what Kubernetes (and hence kubectl) release this corresponds to.

[KnVerey]

That's kinda impossible to say, since we don't have a specific timeline for v5 yet and it will depend on what upstream release is in progress when we do cut it. Should I be more vague instead, i.e. "a future release of Kustomize"?

[sftim]

We avoid statements about the future, unless we can be specific and can commit to the details we're providing.

How about:

The Kustomize tool makes special use of .env files that is different from how most tooling treats this
file.
Typically, a .env file provides default settings for environment variables that you can override, by
setting the actual environment variable. However, Kustomize has a different interpretation. With
Kustomize (and kubectl kustomize), a .env file specifies a set of inputs that you should not
attempt to override by setting the same value in the process environment.

However, there is a bug in kubectl kustomize that occurs when you omit an expected value from an env file.
If you have a .env file that does not specify a value, and you do set the environment variable of the
same name, then the kubectl tool loads that missing value. The authors of Kustomize see this behavior
as a bug.

[sftim]

How about, instead, rewriting the page so that the input filename is not .env (because this filename is associated with a convention that is the opposite of the behavior that Kustomize wants to recommend).

It's unfortunate that the field listing those filenames is envs (and not, eg, parameterPaths or inputs). Perhaps a different change to Kustomize or kubectl would also switch to a new preferred field and deprecate envs?

[sftim]

/sig cli

[sftim]

In terms of timing: we could target v1.25 docs with this change, and then put something into the v1.25 release notes to explain that the previous behavior is now seen as a bug.

It's not a deprecation, exactly, but it's pretty similar in terms of how we'd handle it.

[sftim]

/hold

Pending discussion about target branch, and also about the overall approach.
@KnVerey I recommend you also open an issue and link this PR to that issue.

[natasha41575]

I'm a little bit confused why reverting the docs change is controversial. It was merged without knowledge or approval by the kustomize maintainers and had it been brought to our attention, we would have fixed the bug a long time ago. I don't think it makes sense to continue documenting a bug that is specifically against kustomize's philosophy that we never intended to be used and don't want users to rely on. I'd really prefer that we stick to @KnVerey's original proposal of removing it from the docs entirely because it shouldn't have been put up there to begin with.

Re: which version of kubernetes/kubectl this change will go in, kustomize release is completely independent of kubernetes and kubectl. It doesn't make sense to guarantee a particular kustomize fix will go into any particular kubernetes release; we have no way of predicting it because we are on our own release schedule and can't time it like that.

[sftim]

I'm a little bit confused why reverting the docs change is controversial. It was merged without knowledge or approval by the kustomize maintainers and had it been brought to our attention, we would have fixed the bug a long time ago. I don't think it makes sense to continue documenting a bug that is specifically against kustomize's philosophy that we never intended to be used and don't want users to rely on. I'd really prefer that we stick to @KnVerey's original proposal of removing it from the docs entirely because it shouldn't have been put up there to begin with.

I have read the proposed changes @natasha41575, and I do have concerns still.

This is not a plain revert. If the PR were a pure reversion, I think it'd be more straightforward to review. With that in mind, @KnVerey, you could open a new PR, omitting changes from b4d5224, and link that PR to the issue (yes, you really should open an issue - if we're reverting an accepted change, then as maintainers for the website we would like an issue logged that tracks the motivation for that revert).

Some of the damage is done. I get that. We need to undo that as much as we can, but we also need to accept that people will be accustomed to the now-considered-buggy behavior that has been present for many minor releases, and it's important that we communicate this updated thinking effectively.

I do recommend shifting much of the discussion to that new issue, rather than this PR. Issue discussions are the right place for “how should we fix this” discussions in the large, whereas PR discussions are better for reviews that look like “watch out, the code changes that are proposed here don't quite do what the description says they should”

[sftim]

@KnVerey / other kustomize folks: would you be willing to open that issue to track the work and allow discussion of a way forward that works for different stakeholders?

[KnVerey]

Would you be willing to open that issue to track the work and allow discussion of a way forward that works for different stakeholders?

#35669

This is not a plain revert. If the PR were a pure reversion, I think it'd be more straightforward to review.

This was a straight revert. I added the second commit at the request of the docs team: #35522 (review). I am more than happy to go back to a straight revert.

[sftim]

Thanks for the issue. Let's make this a plain revert for now, and then move the issue forward.

[KnVerey]

Straight revert restored. PTAL @sftim

[sftim]

/lgtm
/hold cancel

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 3e31a3db315d90259f7d7eb7e8679ba21905e7bc

[natalisucks]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: natalisucks

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• content/en/OWNERS [natalisucks]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[joebowbeer]

Fixes #35669 ?

[sftim]

This PR was not a full fix for #35669, in as much as we still need to explain the shipped behavior to end users and #35669 serve s to track that need.