Clean code near `Loader` #4702

annasong20 · 2022-07-08T22:55:17Z

Fix

util.GlobPatternsWithLoader doesn't load remote files
edit add command TestAddKustomizationFileAs* adds normal config instead of kustomization file

Clean

fileloader_test.go, which has 1 invalid check, multiple lengthy checks, and 1 test that's difficult to understand

k8s-ci-robot · 2022-07-08T22:55:25Z

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

natasha41575 · 2022-07-08T22:58:48Z

api/ifc/ifc.go

@@ -28,12 +28,23 @@ type KvLoader interface {

 // Loader interface exposes methods to read bytes.
 type Loader interface {
+
+	// IsRemote returns whether this Loader was created from a url
+	IsRemote() bool


I don't think this is needed. We can make an assumption that if Repo is "", then this wasn't created from a url.

I was thinking this would make future code easier to read. For example, if ldr.IsRemote() reads better than if ldr.Repo() == "".

However, I really like your point. I'll collapse the 2 methods into Repo() string, bool, where the 1st argument is the repo and the second indicates whether there is a repo.

Will address this in #4652

natasha41575 · 2022-07-08T23:06:12Z

api/ifc/ifc.go

+
+	// Repo returns the repo location of this Loader if IsRemote is true,
+	// and an empty string otherwise
+	Repo() string


It would be helpful if you could explain why this is needed and where would you use this. In general, there isn't a big issue with adding methods to our interfaces, but this seems like it could potentially be a field in a struct or a local variable that is passed around. With some more context, we can make a better judgment about the recommendation.

IIRC this change was a prerequisite for #4652, so maybe it makes sense for you to include this change in that PR, as that will provide the context and help reviewers to see how you are planning to use it.

I will move the Repo() change into #4652 and keep auxiliary changes here.

kustomize/commands/internal/util/util.go

natasha41575 · 2022-07-08T23:21:07Z

kustomize/commands/internal/util/util_test.go

-		t.Fatalf("incorrect resources: %v", resources)
-	}
+	resources, err := globPatternsWithLoader(fSys, ldr, []string{httpPath})
+	req.NoError(err)


There seem to be some test changes here that are relevant to the Loader change, and some that aren't. Could you move the extra test fixes to their own PR?

Done. Only the extra fixes remain in this PR. The actual Loader change will go in #4652

Fix and clean code encountered while adding Repo() to Loader

k8s-ci-robot · 2022-08-30T18:36:41Z

@annasong20: PR needs rebase.

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-triage-robot · 2022-11-28T18:54:34Z

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 · 2022-12-28T19:44:30Z

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

yuwenma · 2023-01-11T16:43:58Z

api/internal/git/cloner.go

-// cloneDir field in the repoSpec.  It's assumed that
-// the cloneDir is associated with some fake filesystem
+// the Dir field in the repoSpec.  It's assumed that
+// the Dir is associated with some fake filesystem
 // used in a test.
 func DoNothingCloner(dir filesys.ConfirmedDir) Cloner {


Just realized this. It's an interesting name 😆 . Is this for test only? like "TestOnlyCloner" "ForTestingPurposeOnlyCloner".

k8s-ci-robot · 2023-01-11T16:46:24Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: annasong20, yuwenma
Once this PR has been reviewed and has the lgtm label, please assign knverey for approval by writing /assign @knverey in a comment. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

yuwenma · 2023-01-11T16:46:48Z

/lgtm

@natasha41575 can you take another pass on this PR? It looks good on my side

natasha41575 · 2023-01-30T19:04:44Z

/ok-to-test

k8s-triage-robot · 2023-03-01T19:59:43Z

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

This bot triages 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 PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

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

/close

k8s-ci-robot · 2023-03-01T19:59:48Z

@k8s-triage-robot: Closed this PR.

In response to this:

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

This bot triages 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 PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

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

/close

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 added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 8, 2022

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 8, 2022

k8s-ci-robot requested review from KnVerey and yuwenma July 8, 2022 22:55

natasha41575 reviewed Jul 8, 2022

View reviewed changes

Clean code near Loader

e9e8f7a

Fix and clean code encountered while adding Repo() to Loader

annasong20 force-pushed the remote-loader branch from 813c359 to e9e8f7a Compare July 15, 2022 17:46

annasong20 changed the title ~~Add IsRemote(), Repo() to Loader~~ Clean code near Loader Jul 15, 2022

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2022

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 26, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 28, 2022

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 Dec 28, 2022

yuwenma approved these changes Jan 11, 2023

View reviewed changes

k8s-ci-robot assigned yuwenma Jan 11, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 11, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 30, 2023

k8s-ci-robot closed this Mar 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean code near `Loader` #4702

Clean code near `Loader` #4702

annasong20 commented Jul 8, 2022 •

edited

Loading

k8s-ci-robot commented Jul 8, 2022

natasha41575 Jul 8, 2022

annasong20 Jul 8, 2022

annasong20 Jul 15, 2022

natasha41575 Jul 8, 2022 •

edited

Loading

annasong20 Jul 14, 2022

natasha41575 Jul 8, 2022

annasong20 Jul 15, 2022

k8s-ci-robot commented Aug 30, 2022

k8s-triage-robot commented Nov 28, 2022

k8s-triage-robot commented Dec 28, 2022

yuwenma Jan 11, 2023

k8s-ci-robot commented Jan 11, 2023

yuwenma commented Jan 11, 2023

natasha41575 commented Jan 30, 2023

k8s-triage-robot commented Mar 1, 2023

k8s-ci-robot commented Mar 1, 2023

Clean code near Loader #4702

Clean code near Loader #4702

Conversation

annasong20 commented Jul 8, 2022 • edited Loading

k8s-ci-robot commented Jul 8, 2022

natasha41575 Jul 8, 2022

Choose a reason for hiding this comment

annasong20 Jul 8, 2022

Choose a reason for hiding this comment

annasong20 Jul 15, 2022

Choose a reason for hiding this comment

natasha41575 Jul 8, 2022 • edited Loading

Choose a reason for hiding this comment

annasong20 Jul 14, 2022

Choose a reason for hiding this comment

natasha41575 Jul 8, 2022

Choose a reason for hiding this comment

annasong20 Jul 15, 2022

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 30, 2022

k8s-triage-robot commented Nov 28, 2022

k8s-triage-robot commented Dec 28, 2022

yuwenma Jan 11, 2023

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 11, 2023

yuwenma commented Jan 11, 2023

natasha41575 commented Jan 30, 2023

k8s-triage-robot commented Mar 1, 2023

k8s-ci-robot commented Mar 1, 2023

Clean code near `Loader` #4702

Clean code near `Loader` #4702

annasong20 commented Jul 8, 2022 •

edited

Loading

natasha41575 Jul 8, 2022 •

edited

Loading