search

kubernetes-sigs / kustomize

Customization of kubernetes YAML configurations
Apache License 2.0
10.7k stars 2.22k forks source link

fix: always show accumulation errors #5693

Closed colinodell closed 1 month ago

colinodell commented 1 month ago

For accumulation errors when...

  1. The file load fails due to malformed YAML
  2. The base load succeeds without issues
  3. The directory fails to accumulate

... we will now report the accumulation errors up the stack. Previously, only the malformed YAML error was returned, masking the actual accumulation errors that were the real problem.

Sending just the YAML error makes sense in situations where the base load completely fails and we're pretty confident the base isn't actually a base. But, if we were able to successfully load that base, then the YAML error alone is pretty irrelevant because we know the resource was not a file after all.

Fixes https://github.com/kubernetes-sigs/kustomize/issues/5692

k8s-ci-robot commented 1 month ago

Hi @colinodell. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
k8s-ci-robot commented 1 month ago

This PR has multiple commits, and the default merge method is: merge. You can request commits to be squashed using the label: tide/merge-method-squash

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-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
colinodell commented 1 month ago

Hi @stormqueen1990,

Writing this test was very difficult, especially since kusttarget_test.go (which was supposed to test the YAML error handling) didn't already cover the bottom half of accumulateResources() 😕 But after a few hours I was able to implement something I'm happy with, partially inspired by how accumulation_test.go uses a test server to mock remote responses. Let me know what you think or if further changes are needed here.

Cheers!

colinodell commented 1 month ago

/retest

colinodell commented 1 month ago

/retest

koba1t commented 1 month ago

Thanks @colinodell! /approve

k8s-ci-robot commented 1 month ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: colinodell, koba1t

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: - ~~[OWNERS](https://github.com/kubernetes-sigs/kustomize/blob/master/OWNERS)~~ [koba1t] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment