kustomize icon indicating copy to clipboard operation
kustomize copied to clipboard

Missing error case coverage in api/internal/target/kusttarget.go

Open KnVerey opened this issue 3 years ago • 2 comments

Discovered during review of the remote loader's integration suite revamp.

We are going to lift the code freeze on the remote loader, but will require test coverage improvements along with or ahead of any changes to accumulateResources specifically going forward. This affects Kustomization loading for both remote and local targets.

Part of the problem here is that accumulateResources is making very uneducated guesses about why it got the two possible errors back from functions it called. @annasong20 points out that we could expose new error types in ifc.Loader and take advantage of those to make better decisions on which error to return if they both fail (as well as whether or not to even try the second load in the first place, although we'll need the enhanced coverage of current behaviour in place before we can improve it).

/assign @mightyguava

KnVerey avatar Sep 23 '22 15:09 KnVerey

@KnVerey: GitHub didn't allow me to assign the following users: mightyguava.

Note that only kubernetes-sigs members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. For more information please see the contributor guide

In response to this:

Discovered during review of the remote loader's integration suite revamp.

We are going to lift the code freeze on the remote loader, but will require test coverage improvements along with or ahead of any changes to accumulateResources specifically going forward. This affects Kustomization loading for both remote and local targets.

Part of the problem here is that accumulateResources is making very uneducated guesses about why it got the two possible errors back from functions it called. @annasong20 points out that we could expose new error types in ifc.Loader and take advantage of those to make better decisions on which error to return if they both fail (as well as whether or not to even try the second load in the first place, although we'll need the enhanced coverage of current behaviour in place before we can improve it).

/assign @mightyguava

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 avatar Sep 23 '22 15:09 k8s-ci-robot

lol

/assign @mightyguava

mightyguava avatar Sep 23 '22 16:09 mightyguava

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot avatar Dec 27 '22 22:12 k8s-triage-robot

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot avatar Jan 26 '23 23:01 k8s-triage-robot

Hi @mightyguava, are you still working on this? If not, can I assign it to myself? It is currently blocking #4348.

annasong20 avatar May 08 '23 14:05 annasong20

/assign

annasong20 avatar May 12 '23 17:05 annasong20

@annasong20 can we close this now that your PR is merged? Or is there more work to do here?

natasha41575 avatar Jun 14 '23 16:06 natasha41575

@natasha41575 Still need to add test cases for remote repos, local files, and local directories. Will add a comment in the last test PR that automatically closes this issue.

annasong20 avatar Jun 14 '23 18:06 annasong20

/remove-lifecycle rotten

vaibhav2107 avatar Sep 27 '23 07:09 vaibhav2107

/unassign I haven't had time to work on this in the past few months. Anyone should feel free to pick it up!

annasong20 avatar Dec 04 '23 16:12 annasong20

/assign might have bandwidth to work on this in the next week or so

charles-chenzz avatar Dec 19 '23 13:12 charles-chenzz