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.

Sign up for GitHub

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

Jump to bottom

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

Open
KnVerey opened this issue Sep 23, 2022 · 11 comments
Open

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

KnVerey opened this issue Sep 23, 2022 · 11 comments
Assignees
@charles-chenzz
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/test-coverage Categorizes issue or PR as related to a gap in or problem with our test coverage. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@KnVerey
Copy link
Contributor

KnVerey commented Sep 23, 2022

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 KnVerey added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Sep 23, 2022
@k8s-ci-robot
Copy link
Contributor

@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.

@KnVerey KnVerey mentioned this issue Sep 23, 2022
Closed
@mightyguava
Copy link
Contributor

lol

/assign @mightyguava

@annasong20 annasong20 mentioned this issue Sep 23, 2022
Closed
@KnVerey KnVerey added the kind/test-coverage Categorizes issue or PR as related to a gap in or problem with our test coverage. label Sep 28, 2022
@KnVerey KnVerey mentioned this issue Sep 28, 2022
Closed
@annasong20 annasong20 mentioned this issue Dec 7, 2022
Merged
@annasong20 annasong20 mentioned this issue Dec 21, 2022
Closed
@k8s-triage-robot
Copy link

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-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 27, 2022
@k8s-triage-robot
Copy link

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-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 26, 2023
@annasong20
Copy link
Contributor

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

@annasong20
Copy link
Contributor

/assign

annasong20 added a commit to annasong20/kustomize that referenced this issue May 20, 2023
@annasong20
Add accumulateResources error coverage tests
76fc382 
Test accumulateResources errors when a remote file fails to load. This is part of the effort to fix issue kubernetes-sigs#4807.
@annasong20 annasong20 mentioned this issue May 20, 2023
Merged
annasong20 added a commit to annasong20/kustomize that referenced this issue May 31, 2023
@annasong20
Add accumulateResources error coverage tests
8c0462d 
Test accumulateResources errors when a remote file fails to load. This is part of the effort to fix issue kubernetes-sigs#4807.
annasong20 added a commit to annasong20/kustomize that referenced this issue Jun 9, 2023
@annasong20
Add accumulateResources remote file error tests
168e31b 
Test accumulateResources errors when a remote file fails to load. This
is part of the effort to fix issue kubernetes-sigs#4807.
@natasha41575
Copy link
Contributor

natasha41575 commented Jun 14, 2023 •
edited

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

@annasong20
Copy link
Contributor

@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 added a commit to annasong20/kustomize that referenced this issue Jun 28, 2023
@annasong20
Add accumulateResources error tests for local files.
710d909 
Add tests demonstrating accumulateResources errors when the resource is
a local file. Works to fix kubernetes-sigs#4807.
@annasong20 annasong20 mentioned this issue Jun 28, 2023
Merged
annasong20 added a commit to annasong20/kustomize that referenced this issue Jun 28, 2023
@annasong20
Add accumulateResources error tests for local files.
d03abc4 
Add tests demonstrating accumulateResources errors when the resource is
a local file. Works to fix kubernetes-sigs#4807.
annasong20 added a commit to annasong20/kustomize that referenced this issue Jun 28, 2023
@annasong20
Add accumulateResources error tests for local files.
f84afda 
Add tests demonstrating accumulateResources errors when the resource is
a local file. Works to address kubernetes-sigs#4807.
k8s-ci-robot pushed a commit that referenced this issue Jul 1, 2023
@annasong20
Add accumulateResources error tests for local files (#5225)
da4e881 
* Add accumulateResources error tests for local files.

Add tests demonstrating accumulateResources errors when the resource is
a local file. Works to address #4807.

* Improve readability
This was referenced Aug 2, 2023
Open
Open
@annasong20 annasong20 mentioned this issue Aug 11, 2023
Closed
@vaibhav2107
Copy link

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Sep 27, 2023
@annasong20
Copy link
Contributor

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

@charles-chenzz
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@charles-chenzz charles-chenzz

Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/test-coverage Categorizes issue or PR as related to a gap in or problem with our test coverage. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Kustomize Core Usability Bugs
Status: Todo
Milestone
No milestone
Development

No branches or pull requests
8 participants
@mightyguava @KnVerey @natasha41575 @k8s-ci-robot @annasong20 @k8s-triage-robot @charles-chenzz @vaibhav2107