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

Add accumulateResources error tests for local files #5225

Merged
merged 2 commits into from
Jul 1, 2023
Merged

Add accumulateResources error tests for local files #5225

merged 2 commits into from
Jul 1, 2023

Conversation

annasong20
Copy link
Contributor

This is part of the effort to address #4807.

This PR tests accumulateResources error messages for resources that are faulty local files.

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 28, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: annasong20

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label 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 k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jun 28, 2023
@annasong20 annasong20 marked this pull request as ready for review June 28, 2023 17:24
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 28, 2023
@koba1t
Copy link
Member

koba1t commented Jun 28, 2023

/assign

koba1t
koba1t reviewed Jun 28, 2023
View reviewed changes
api/krusty/accumulation_test.go
errFile, errDir string
}
populateAbsolutePaths := func(tc testcase, dir string) testcase {
filePaths := make(map[string]string, len(tc.files)+1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this len and range of tc.files are the same numerics.
Are you need +1 for len(tc.files) for any reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this line of code: https://github.com/kubernetes-sigs/kustomize/pull/5225/files#diff-15a9eb3d3d62fd27ecea240ff711d887220b80ac54f3f709a7ed8bc25632a837R195 Does this make it easier to understand?

In addition to all the files we need to create specified by the test case, we also need to create the build root or the direct kustomization that we run kustomize build on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, This is my misunderstanding.
Thanks for explaining that!

api/loader/fileloader_test.go Outdated
_, err := makeLoader().New("https://google.com/project")
require.Error(t, err)
func TestNewRemoteLoaderDoesNotExist(t *testing.T) {
_, err := makeLoader().New("https://example.com/project")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will be easy to understand to use the same URL for getting the same reason.
https://github.com/kubernetes-sigs/kustomize/pull/5225/files#diff-15a9eb3d3d62fd27ecea240ff711d887220b80ac54f3f709a7ed8bc25632a837R224.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really great catch! I've changed the root url.

I did a little digging, and it turns out I blindly copied this existing test that was running incorrectly. The history of this test is as follows:

  1. This test existed before my previous PR, which only moved it.
  2. This test existed at a time when any http url was considered a repo

Hence, this test originally wanted to check that New() failed when the repo url didn't exist. I assume someone forgot to update this test when they updated api/internal/git/repospec.

To defend against this, I check in the error that the loader tried to fetch the url.

The url is not exactly the same as in the accumulation_test because there I want to clarify where a url is treated as a remote file vs. repo, whereas I don't think we need that here since we explicitly call New(). I'm also happy to change this on further feedback, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for investigating!

api/loader/fileloader_test.go
@@ -635,6 +661,8 @@ func TestLoaderHTTP(t *testing.T) {

// setupOnDisk sets up a file system on disk and directory that is cleaned after
// test completion.
// TODO(annasong): Move all loader tests that require real file system into
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like, according to your comment, you have a plan to remove the setupOnDisk() function and replace it with the Setup() function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Essentially, yeah. I plan to move all tests calling setupOnDisk into krusty/remoteloader_test.go (will rename to loader_test.go), then replace calls to setupOnDisk with Setup.

I can't replace setupOnDisk with Setup in fileloader_test.go because it would create a cyclic dependency.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a great plan!

@k8s-ci-robot
Copy link
Contributor

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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@annasong20
Copy link
Contributor Author

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jul 1, 2023
@koba1t
Copy link
Member

koba1t commented Jul 1, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 1, 2023
@k8s-ci-robot k8s-ci-robot merged commit da4e881 into kubernetes-sigs:master Jul 1, 2023
8 checks passed
@annasong20 annasong20 deleted the test-accumulate-local-files branch July 5, 2023 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@koba1t koba1t koba1t left review comments

@natasha41575 natasha41575 Awaiting requested review from natasha41575

@yuwenma yuwenma Awaiting requested review from yuwenma

Assignees

@koba1t koba1t

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@annasong20 @k8s-ci-robot @koba1t