Refactor repospec code to be more readable

[annasong20]

Describe the bug

RepoSpec readability issues include:

• The function parseGitURL in RepoSpec currently has 7 return values. This makes differentiating between the return values more difficult than it needs to be and prone to error. Potential solutions:

  • return a RepoSpec instead of the 7 fields of RepoSpec
  • parse different parts of the url by calling several helper functions in NewRepoSpecFromURL directly

  An example of this can be found here.

• Rename OrgRepo given reasoning: Fix RepoSpec query extraction #4863 (comment)

• Rename Host, as suggested in: Reinstate #4652 without url-related code #4779 (comment)

  • Rename SchemeAuth
  • Split into multiple fields Scheme, UserInfo, Host, Port, which gives calling code, like kustomize localize, more information

• Decide how to handle url.Parse() error for orgRepo, Path, and query, as suggested here: Fix RepoSpec query extraction #4863 (comment)

Version
Kustomize master branch, commit e724e25

[annasong20]

/label kind/cleanup

[k8s-ci-robot]

@annasong20: The label(s) /label kind/cleanup cannot be applied. These labels are supported: api-review, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, team/katacoda, refactor. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to this:

/label kind/cleanup

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]

/kind cleanup

[KnVerey]

/triage accepted

[natasha41575]

I think we should make this issue an umbrella issue to track the refactoring we want to do for repoSpec. WDYT about retitling it to "Refactor repospec code to be more readable" and listing this particular issue as one of the things we want to do? Then we can add more to this issue as we go along, such as the suggestion here #4863 (comment) - rather than filing a separate issue for each thing.

[natasha41575]

@koba1t would you be interested in taking this issue?

[kishorerj]

@annasong20 @natasha41575 I am looking at starting my contribution journey in K8 and would like to pick this up.. Can u please assign it to me?

[kishorerj]

/assign

[koba1t]

Hello @kishorerj!
Thanks for taking this issue!
We're waiting for your PR!

[kishorerj]

@koba1t @annasong20 @natasha41575 I created the draft PR with changes to the first task mentioned in the issue above (replace 7 return values with repo spec). #4891. I built the code and also ran the tests to ensure there are no impacts.. Since this is my first change, I have to do a few pending items on CLA which I will get sorted soon. Also will be committing changes for the other issue tasks mentioned here

[kishorerj]

@annasong20 @natasha41575 I have created PR for the first task as mentioned above, please take a look at it. I need more details for the second task. What are we trying to achieve here. An example will help.. Also whats the best way to get connected on this issue? Do i ping in slack, set up a zoom meeting?

[annasong20]

@kishorerj and I synced offline. I edited the issue with examples and more context.

[kishorerj]

@kishorerj and I synced offline. I edited the issue with examples and more context.

Thanks Anna for giving more context.. This helps

[kishorerj]

@annasong20 @natasha41575 I have opened this PR for review(closed the earlier PR I created).. #4900.
It has fixes for Task1 that you have mentioned in this issue.. I will open seperate PRs for other tasks

[kishorerj]

@koba1t @annasong20 @natasha41575 I created the draft PR with changes to the first task mentioned in the issue above (replace 7 return values with repo spec). #4891. I built the code and also ran the tests to ensure there are no impacts.. Since this is my first change, I have to do a few pending items on CLA which I will get sorted soon. Also will be committing changes for the other issue tasks mentioned here

Please ignore this PR.. I have closed it.. Please consider #4900

[kishorerj]

@KnVerey @natasha41575 @annasong20 For2nd task any suggestions on what could be the name instead of OrgRepo ? .. How about repo or repoRoot or repoRootPath .. ? Any opinion?

Again on 4th , the url.parsePath() did throw an error with % at the end when I tried as mentioned.. Since I am not sure about the actual context in which this is used further up the stack, is this fine to even proceed by setting the path to "" and logging a warning or do we throw an error and stop any subsequent processing of this url as the url itself is invalid and ask the user to fix this?

[annasong20]

2. I don't have a strong preference. Maybe repoPath for OrgRepo and rootPath for Path? Will defer to @KnVerey, @natasha41575
3. I'm leaning towards returning an error. In the case we're parsing an OrgRepo, we'll be returning an error anyway if we set it to the empty string. In the case of Path, I find it strange to treat the repo as the target root if we can't parse the path. Granted, the error might not result from an invalid OrgRepo or Path; maybe url.Parse just has a different spec than that of acceptable OrgRepos and Paths for certain edge cases. However, until we stop using url.Parse to extract OrgRepo, Path, we should return the error to signify to users we don't know how to handle the input. I also don't have all context, so again will defer to @KnVerey, @natasha41575

[KnVerey]

2. I don't have a strong preference. Maybe repoPath for OrgRepo and rootPath for Path?

I like repoPath for OrgRepo. For Path, I suggest KustRootPath to clarify that it isn't the repo or clone root.

4. I'm leaning towards returning an error. In the case we're parsing an OrgRepo, we'll be returning an error anyway if we set it to the empty string. In the case of Path, I find it strange to treat the repo as the target root if we can't parse the path. Granted, the error might not result from an invalid OrgRepo or Path; maybe url.Parse just has a different spec than that of acceptable OrgRepos and Paths for certain edge cases. However, until we stop using url.Parse to extract OrgRepo, Path, we should return the error to signify to users we don't know how to handle the input.

My instinct is also to return the error, though it is possible the larger test suite (I only ran a subset previously) might reveal a reason not to do this. Let's try it and discuss further on the PR if needed.

[kishorerj]

@annasong20 @natasha41575 @KnVerey pushed the changes for name changes in the PR #4922. I am not clear on task3 . Next I will pick up task 4 on handling url.parse() error and try to push this on Monday/Tuesday