git submodule need origin #5353

charles-chenzz · 2023-10-01T08:31:15Z

antoooks · 2023-10-03T13:42:40Z

Hi @charles-chenzz, I checked out to the branch and tested it out using go test with URL provided by the issue creator https://github.com/etc/etc/etc?ref=deadbeefdeafbeef it seems to parse the host incorrectly. Seems the parser does not support parsing path with more than 2 level of subpath? please find the details below:

=== RUN   TestNewRepoSpecFromUrl_Smoke/arbitrary_testing
    repospec_test.go:699: https://github.com/etc/etc
    repospec_test.go:702: 
                Error Trace:    /Volumes/Workstation/Project/kustomize/api/internal/git/repospec_test.go:702
                Error:          Not equal: 
                                expected: "/notCloned"
                                actual  : "/notCloned/etc"
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -/notCloned
                                +/notCloned/etc
                Test:           TestNewRepoSpecFromUrl_Smoke/arbitrary_testing
                Messages:       absPath mismatch
    repospec_test.go:710: 
                Error Trace:    /Volumes/Workstation/Project/kustomize/api/internal/git/repospec_test.go:710
                Error:          Not equal: 
                                expected: &git.RepoSpec{raw:"", Host:"https://gist.github.com/", RepoPath:"bc7947cb727d7f9217e7862d961a1ffd.git", Dir:"", KustRootPath:"", Ref:"", Submodules:false, Timeout:0}
                                actual  : &git.RepoSpec{raw:"", Host:"https://github.com/", RepoPath:"etc/etc", Dir:"", KustRootPath:"etc", Ref:"deadbeefdeafbeef", Submodules:false, Timeout:0}
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -2,7 +2,7 @@
                                  raw: (string) "",
                                - Host: (string) (len=24) "https://gist.github.com/",
                                - RepoPath: (string) (len=36) "bc7947cb727d7f9217e7862d961a1ffd.git",
                                + Host: (string) (len=19) "https://github.com/",
                                + RepoPath: (string) (len=7) "etc/etc",
                                  Dir: (filesys.ConfirmedDir) "",
                                - KustRootPath: (string) "",
                                - Ref: (string) "",
                                + KustRootPath: (string) (len=3) "etc",
                                + Ref: (string) (len=16) "deadbeefdeafbeef",
                                  Submodules: (bool) false,
                Test:           TestNewRepoSpecFromUrl_Smoke/arbitrary_testing

My go test file content:

// api/internal/git/repospec_test.go#683

{
	name:      "arbitrary testing",
	input:     "https://github.com/etc/etc/etc?ref=deadbeefdeafbeef",
	cloneSpec: "https://github.com/etc/etc",
	absPath:   notCloned.String(),
	repoSpec: RepoSpec{
		Host:     "https://gist.github.com/",
		RepoPath: "bc7947cb727d7f9217e7862d961a1ffd.git",
	},
},

charles-chenzz · 2023-10-03T14:16:15Z

My go test file content:

{
  name:      "arbitrary testing",
  input:     "https://github.com/etc/etc/etc?ref=deadbeefdeafbeef",
  cloneSpec: "https://github.com/etc/etc",
  absPath:   notCloned.String(),
  repoSpec: RepoSpec{
  	Host:     "https://gist.github.com/",
  	RepoPath: "bc7947cb727d7f9217e7862d961a1ffd.git",
  },
},

this testcase was on the testfile or you create it? I didn't quite get the point here ( might need detail explanation sorry)
there's a regression test already add by anna.

antoooks · 2023-10-03T14:54:40Z

My go test file content:
{
  name:      "arbitrary testing",
  input:     "https://github.com/etc/etc/etc?ref=deadbeefdeafbeef",
  cloneSpec: "https://github.com/etc/etc",
  absPath:   notCloned.String(),
  repoSpec: RepoSpec{
  	Host:     "https://gist.github.com/",
  	RepoPath: "bc7947cb727d7f9217e7862d961a1ffd.git",
  },
},
this testcase was on the testfile or you create it? I didn't quite get the point here ( might need detail explanation sorry) there's a regression test already add by anna.

yes, I created one more test case on the existing test file using the URL provided on the issue. Specifically starting on line#683. I did so because I noticed that the log message provided by the issue creator seems off, I'm not sure if that's a typo though.

Error: accumulating resources: accumulation err='accumulating resources from 'https://github.cometc/etc/etc?ref=deadbeefdeafbeef&timeout=90s': URL is a git repository': failed to run '/usr/bin/git submodule update --init --recursive': warning: could not look up configuration 'remote.origin.url'. Assuming this repository is its own authoritative upstream.

charles-chenzz · 2023-10-04T03:15:22Z

kustomize/api/internal/git/repospec_test.go

Lines 153 to 155 in fb7ee2f

    
           // A set of end to end parsing tests that smoke out obvious issues 
        
           // No tests for submodules and timeout as the expectations are hard-coded 
        
           // to the defaults for compactness.

kustomize/api/internal/git/repospec_test.go

Lines 689 to 691 in fb7ee2f

    
           // some values have defaults. Clear them here so test cases remain compact. 
        
           // This means submodules and timeout cannot be tested here. That's fine since 
        
           // they are tested in TestParseQuery.

base on the comments here, I think that's why submodules testcase won't work here. (don't know if I understand it right here)

the regression test is introduce in this #5189

antoooks · 2023-10-04T05:00:50Z

kustomize/api/internal/git/repospec_test.go

Lines 153 to 155 in fb7ee2f

// A set of end to end parsing tests that smoke out obvious issues

// No tests for submodules and timeout as the expectations are hard-coded

// to the defaults for compactness.

kustomize/api/internal/git/repospec_test.go

Lines 689 to 691 in fb7ee2f

// some values have defaults. Clear them here so test cases remain compact.

// This means submodules and timeout cannot be tested here. That's fine since

// they are tested in TestParseQuery.

base on the comments here, I think that's why submodules testcase won't work here. (don't know if I understand it right here)

the regression test is introduce in this #5189

Hm that's unfortunate, since the issue OP also didn't include reproduce steps as well. Other changes seems aligned with the proposed solution. I think it's good to go after you update this README to make it stay relevant.

https://github.com/charles-chenzz/kustomize/blob/4b225f68c57a23bf507fa50d19c4326078cebd21/api/krusty/testdata/remoteload/with-submodule/README.md#L8-L10

charles-chenzz · 2023-10-04T12:04:39Z

I think it's good to go after you update this README to make it stay relevant.

the stay relevant means remove the comment in https://github.com/charles-chenzz/kustomize/blob/4b225f68c57a23bf507fa50d19c4326078cebd21/api/krusty/testdata/remoteload/with-submodule/README.md#L8-L10 ? I'm not quite get the points here. sorry

antoooks · 2023-10-04T12:06:03Z

I think it's good to go after you update this README to make it stay relevant.

the stay relevant means remove the comment in https://github.com/charles-chenzz/kustomize/blob/4b225f68c57a23bf507fa50d19c4326078cebd21/api/krusty/testdata/remoteload/with-submodule/README.md#L8-L10 ? I'm not quite get the points here. sorry

Yup, right, you can update it since #5131 has been resolved, after this PR is merged. 😃

charles-chenzz · 2023-10-04T12:13:25Z

Yup, right, you can update it since #5131 has been resolved, after this PR is merged

ACK. I will let other reviewers opt-in and see if there's anything need to address, would update it on another commit/PR

antoooks · 2023-10-04T12:14:20Z

/lgtm from me

jzelinskie · 2023-10-05T20:57:24Z

I work with @jakedt -- who reported this issue.
I confirmed that this branch resolves the issue for us.

Thanks!

annasong20

@charles-chenzz Looks great! Just a few nits that I think will make the code easier to read for future developers.

/lgtm

api/internal/git/cloner.go

annasong20 · 2023-10-06T20:14:52Z

/hold

annasong20 · 2023-10-06T20:47:00Z

@charles-chenzz Sorry for the confusion. I didn't know that Prow automatically adds the approve label on lgtm. I will remove the lgtm until we address the nits.

/lgtm cancel
/remove-approve
/unhold

annasong20 · 2023-10-06T22:00:27Z

@antoooks Regarding your earlier comment,

it seems to parse the host incorrectly. Seems the parser does not support parsing path with more than 2 level of subpath?
// api/internal/git/repospec_test.go#683

{
name: "arbitrary testing",
input: "https://github.com/etc/etc/etc?ref=deadbeefdeafbeef",
cloneSpec: "https://github.com/etc/etc",
absPath: notCloned.String(),
repoSpec: RepoSpec{
Host: "https://gist.github.com/",
RepoPath: "bc7947cb727d7f9217e7862d961a1ffd.git",
},
},

The parser does support multi-level sub-paths.

I think the problem lies in the test case. You can also see this in the test output you provided. For example, you provider a github.com input, but expect the parsed RepoSpec to have host gist.github.com. These two domains don't match. As another example, by default the parser takes the first 2 segments after the domain as the path to the repo. That would mean that your 3rd and last etc specifies the path to the Kustomization within the repo. Therefore, your absPath should include an etc and your RepoSpec.RepoPath should also be etc.

And with regards to

I did so because I noticed that the log message provided by the issue creator seems off, I'm not sure if that's a typo though.

Sharp eye :) No, the lack of a slash in the original issue description is not Kustomize's doing. I hypothesize that in order to generalize his url, the reporter whited out his own url with etc and in the process, deleted an extra /

annasong20 · 2023-10-06T22:05:19Z

I think it's good to go after you update this README to make it stay relevant.

the stay relevant means remove the comment in https://github.com/charles-chenzz/kustomize/blob/4b225f68c57a23bf507fa50d19c4326078cebd21/api/krusty/testdata/remoteload/with-submodule/README.md#L8-L10 ? I'm not quite get the points here. sorry

I'd prefer we not remove this explanation. It doesn't say that Kustomize still contains the bug, only that the hash is a preventative measure against such a bug. It explains why we include a hash, which a new contributor might wonder in the future. You can clarify the sentence if you'd like, but I'd only delete it if you plan to remove the hash from the testing code, which I think is slightly undesirable because we should be careful when testing locally on developers' machines.

charles-chenzz · 2023-10-06T23:52:13Z

@annasong20 I've addressed the nits, could you revisit when you get a chance?

@charles-chenzz Sorry for the confusion. I didn't know that Prow automatically adds the approve label on lgtm. I will remove the lgtm until we address the nits.

prow won't automatically add the approve label after lgtm maybe it's bug here..

annasong20 · 2023-10-07T01:16:06Z

/lgtm

annasong20 · 2023-10-07T01:23:27Z

/approve

annasong20

@annasong20 I've addressed the nits, could you revisit when you get a chance?

Looks great! Thanks, @charles-chenzz!!

@charles-chenzz Sorry for the confusion. I didn't know that Prow automatically adds the approve label on lgtm. I will remove the lgtm until we address the nits.

prow won't automatically add the approve label after lgtm maybe it's bug here..

I just tested again and my lgtm didn't automatically add an approve. I am thoroughly confused. Would investigate if I had more time.

k8s-ci-robot · 2023-10-07T01:26:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: annasong20, charles-chenzz

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~~ [annasong20]

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 added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 1, 2023

k8s-ci-robot requested review from annasong20 and yuwenma October 1, 2023 08:31

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 1, 2023

natasha41575 assigned annasong20 Oct 6, 2023

annasong20 suggested changes Oct 6, 2023

View reviewed changes

api/internal/git/cloner.go Outdated Show resolved Hide resolved

api/internal/git/cloner.go Show resolved Hide resolved

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 6, 2023

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 6, 2023

k8s-ci-robot removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 6, 2023

add orgin back when clone repo (due to git submodule).

60d7ee6

charles-chenzz force-pushed the git_submodule branch from 4b225f6 to 60d7ee6 Compare October 6, 2023 23:50

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 6, 2023

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 7, 2023

annasong20 approved these changes Oct 7, 2023

View reviewed changes

k8s-ci-robot merged commit 779f153 into kubernetes-sigs:master Oct 7, 2023
5 checks passed

charles-chenzz deleted the git_submodule branch October 7, 2023 01:56

sarab97 mentioned this pull request Oct 11, 2023

Version 5.0.0 breaks referencing a remote repo containing git LFS files #5355

Closed

ncapps mentioned this pull request Dec 13, 2023

Document API Specs in Reference section #5379

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

git submodule need origin #5353

git submodule need origin #5353

charles-chenzz commented Oct 1, 2023

antoooks commented Oct 3, 2023 •

edited

Loading

charles-chenzz commented Oct 3, 2023 •

edited

Loading

antoooks commented Oct 3, 2023 •

edited

Loading

charles-chenzz commented Oct 4, 2023 •

edited

Loading

antoooks commented Oct 4, 2023 •

edited

Loading

charles-chenzz commented Oct 4, 2023

antoooks commented Oct 4, 2023 •

edited

Loading

charles-chenzz commented Oct 4, 2023

antoooks commented Oct 4, 2023

jzelinskie commented Oct 5, 2023

annasong20 left a comment

annasong20 commented Oct 6, 2023

annasong20 commented Oct 6, 2023

annasong20 commented Oct 6, 2023

annasong20 commented Oct 6, 2023

charles-chenzz commented Oct 6, 2023

annasong20 commented Oct 7, 2023

annasong20 commented Oct 7, 2023

annasong20 left a comment

k8s-ci-robot commented Oct 7, 2023

git submodule need origin #5353

git submodule need origin #5353

Conversation

charles-chenzz commented Oct 1, 2023

antoooks commented Oct 3, 2023 • edited Loading

charles-chenzz commented Oct 3, 2023 • edited Loading

antoooks commented Oct 3, 2023 • edited Loading

charles-chenzz commented Oct 4, 2023 • edited Loading

antoooks commented Oct 4, 2023 • edited Loading

charles-chenzz commented Oct 4, 2023

antoooks commented Oct 4, 2023 • edited Loading

charles-chenzz commented Oct 4, 2023

antoooks commented Oct 4, 2023

jzelinskie commented Oct 5, 2023

annasong20 left a comment

Choose a reason for hiding this comment

annasong20 commented Oct 6, 2023

annasong20 commented Oct 6, 2023

annasong20 commented Oct 6, 2023

annasong20 commented Oct 6, 2023

charles-chenzz commented Oct 6, 2023

annasong20 commented Oct 7, 2023

annasong20 commented Oct 7, 2023

annasong20 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 7, 2023

antoooks commented Oct 3, 2023 •

edited

Loading

charles-chenzz commented Oct 3, 2023 •

edited

Loading

antoooks commented Oct 3, 2023 •

edited

Loading

charles-chenzz commented Oct 4, 2023 •

edited

Loading

antoooks commented Oct 4, 2023 •

edited

Loading

antoooks commented Oct 4, 2023 •

edited

Loading