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.

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

Implement locRootPath #4909

Merged
merged 3 commits into from
Dec 22, 2022
Merged

Conversation

annasong20
Copy link
Contributor

This PR

  • implements locRootPath, which calculates the localized path for remote roots
  • changes the behavior of locFilePath to include userinfo and port in the path, and adds tests

We include userinfo and port now because otherwise we'd need url.Parse to remove them in locRootPath. Also, as discussed with @natasha41575, because the file scheme contains absolute paths on the local file system instead of url authorities, we put them inside their own sub-directory inside the localize directory.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 6, 2022
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 6, 2022
@annasong20
Copy link
Contributor Author

/cc @natasha41575

Copy link
Contributor

@natasha41575 natasha41575 left a comment

Choose a reason for hiding this comment

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

Question, what value does including userinfo in the resulting directory structure provide? To me it seems unnecessary to keep it, but I'd love to hear your opinion. How difficult is it to remove with url.Parse?

@annasong20
Copy link
Contributor Author

annasong20 commented Dec 9, 2022

Question, what value does including userinfo in the resulting directory structure provide? To me it seems unnecessary to keep it, but I'd love to hear your opinion. How difficult is it to remove with url.Parse?

Different userinfo can technically lead to different remote content. I'm not 100% certain, but I believe the idea is that userinfo indicates the user authenticating the domain, and each user can see a different view of the domain (edge case). For example, if the git repo path contains the working directory, i.e. ssh://user@host.com/~/repo.git, different users will have different working directories with different repos under path ~/repo.

My initial motivation for including userinfo in this PR, despite us having decided to remove it for simplicity and consolidation of https://github.com, ssh://git@github.com, and git@github.com urls, was to avoid parsing it out from remote roots. (I realized my previous approach of blindly parsing based on @ and : characters was flawed.) However, a valid workaround is to use url.Parse to parse userinfo and port for the remote roots too, so we could totally remove userinfo and port if we wanted to.

userinfo is an edge case that shouldn't be very common; we just have to decide whether we want to support it.

@natasha41575
Copy link
Contributor

I'd prefer for now to stick to the KEP and exclude port and userinfo to keep the directory looking simple. It will be easier to add extra support for these edge cases later than it would be to remove them.

@k8s-ci-robot
Copy link
Contributor

@annasong20: 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 Dec 13, 2022
Copy link
Contributor

@yuwenma yuwenma left a comment

Choose a reason for hiding this comment

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

PR looks great on my side! Since we have a lot of discussion about this repoSpec behaviors, I would really prefer another eye on the review for the parseHost method. 

default:
// For, relative ssh, or scp-like syntax, we attach a scheme
// to avoid url.Parse errors
target = "ssh://" + repoSpec.Host
Copy link
Contributor

@natasha41575 natasha41575 Dec 16, 2022

Choose a reason for hiding this comment

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

I'm not that familiar with URL parsing, so I have some questions that may help me understand the goals of this code:

  1. Does this ssh you're adding show up in the output of localize?
  2. Is this ssh a placeholder just to avoid url.Parse errors, or are we intentionally adding the ssh because we expect by default schemes to be ssh://? If it's the first, I wonder if there is some other placeholder we can use that isn't actually a real scheme to make it extra clear.
  3. What are some cases that would hit the default case? Seems like we would hit it for both empty schemes and for other schemes that we don't know about. For the latter (other schemes we don't know about), it seems weird to prepend ssh://. Was that your intention?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. No, we strip the scheme when we call url.Hostname: https://github.com/kubernetes-sigs/kustomize/pull/4909/files/b235147079b094ea1dd8c4ebbb1863c2a2c7dfe6#diff-f52a1bf07c678854203647b33ccaa09d945af03c9233b3dec3a947ba27d531deR212
  2. Yes, it's just to avoid url.Parse errors. However, the correct scheme to add here is technically ssh. See my response to 3.
  3. In this function, git.NewRepoSpecFromURL must have been called successfully. Given this, the only url that hits the default case is the relative ssh case that begins with git@. Relative ssh urls have a different delimiter than full ssh urls (ones with ssh:// header), but we remove this distinction here: https://github.com/kubernetes-sigs/kustomize/pull/4909/files/b235147079b094ea1dd8c4ebbb1863c2a2c7dfe6#diff-f52a1bf07c678854203647b33ccaa09d945af03c9233b3dec3a947ba27d531deR206. Thus, we can safely add the ssh:// scheme header to urls under the default case.

Copy link
Contributor

@natasha41575 natasha41575 left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 22, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: annasong20, natasha41575, yuwenma

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 Dec 22, 2022
@k8s-ci-robot k8s-ci-robot merged commit aec3500 into kubernetes-sigs:master Dec 22, 2022
@annasong20 annasong20 deleted the locRootPath branch December 22, 2022 21:51
kishorerj pushed a commit to kishorerj/kustomize that referenced this pull request Jan 8, 2023
* Implement locRootPath, and include userinfo, port in locFilePath

* Strip userinfo, port

* Improve readability
k8s-ci-robot pushed a commit that referenced this pull request Jan 9, 2023
* initial changes to rename OrgRepo to RepoPath

* changes to rename Path to KustRootPath

* addressed review comments

* addressed review comments

* docs: Add documentation for namespace transformer

Add a short description of the namespace transformer and example
usage to examples/transformerconfigs/README.md.

References: #629
Signed-off-by: Lars Kellogg-Stedman <lars@oddbit.com>

* Localize patchesJson6902, patchesStrategicMerge, replacements (#4904)

* Localize patchesJson6902, patchesStrategicMerge, replacements

* Address code review feedback

* Improve readability
* Remove deprecation warning check

* Load legacy kustomization fields for `localize` (#4918)

* Load legacy kustomization

* Expose loadKustFile in kusttarget

* remove FixKustomizationPreUnmarshalling

* remove deprecated cfg and fn commands (#4930)

* remove deprecated cfg and fn commands

* fix lint error

* run gofmt

* Localize PatchTransformer, PatchJson6902Transformer (#4920)

* Localize patches, patchesJson6902 custom transformers

* Improve readability

* Localize fields: openapi, configurations, crds (#4907)

* Localize openapi, configurations, crds

* Add integration test

* Move krusty test

* Address code review feedback

* Implement locRootPath (#4909)

* Implement locRootPath, and include userinfo, port in locFilePath

* Strip userinfo, port

* Improve readability

* Localize legacy fields

* Localize resources (#4912)

* Localize resources

* Improve readability

* Add integration tests

* Group test helper functions

* Remove Functionality that Pulls Env Variables from Empty Keys

* Update api/kv/kv.go

Co-authored-by: Katrina Verey <kn.verey@gmail.com>

* refactor Unmarshal Kustomization struct code

* improve error messages

* Run go mod tidy on all modules before update

* Update sigs.k8s.io/yaml to 1.3.0

* fixed test failure because of latest commits

Signed-off-by: Lars Kellogg-Stedman <lars@oddbit.com>
Co-authored-by: Lars Kellogg-Stedman <lars@oddbit.com>
Co-authored-by: Anna Song <annasong@google.com>
Co-authored-by: yugo kobayashi <kobdotsh@gmail.com>
Co-authored-by: Natasha Sarkar <natashasarkar@google.com>
Co-authored-by: Cailyn Edwards <cailyn.edwards@shopify.com>
Co-authored-by: Cailyn <cailyn.s.e@gmail.com>
Co-authored-by: Katrina Verey <kn.verey@gmail.com>
Co-authored-by: Katrina Verey <katrina.verey@shopify.com>
cailyn-codes pushed a commit to cailyn-codes/kustomize that referenced this pull request Jan 12, 2023
* Implement locRootPath, and include userinfo, port in locFilePath

* Strip userinfo, port

* Improve readability
cailyn-codes added a commit to cailyn-codes/kustomize that referenced this pull request Jan 12, 2023
* initial changes to rename OrgRepo to RepoPath

* changes to rename Path to KustRootPath

* addressed review comments

* addressed review comments

* docs: Add documentation for namespace transformer

Add a short description of the namespace transformer and example
usage to examples/transformerconfigs/README.md.

References: kubernetes-sigs#629
Signed-off-by: Lars Kellogg-Stedman <lars@oddbit.com>

* Localize patchesJson6902, patchesStrategicMerge, replacements (kubernetes-sigs#4904)

* Localize patchesJson6902, patchesStrategicMerge, replacements

* Address code review feedback

* Improve readability
* Remove deprecation warning check

* Load legacy kustomization fields for `localize` (kubernetes-sigs#4918)

* Load legacy kustomization

* Expose loadKustFile in kusttarget

* remove FixKustomizationPreUnmarshalling

* remove deprecated cfg and fn commands (kubernetes-sigs#4930)

* remove deprecated cfg and fn commands

* fix lint error

* run gofmt

* Localize PatchTransformer, PatchJson6902Transformer (kubernetes-sigs#4920)

* Localize patches, patchesJson6902 custom transformers

* Improve readability

* Localize fields: openapi, configurations, crds (kubernetes-sigs#4907)

* Localize openapi, configurations, crds

* Add integration test

* Move krusty test

* Address code review feedback

* Implement locRootPath (kubernetes-sigs#4909)

* Implement locRootPath, and include userinfo, port in locFilePath

* Strip userinfo, port

* Improve readability

* Localize legacy fields

* Localize resources (kubernetes-sigs#4912)

* Localize resources

* Improve readability

* Add integration tests

* Group test helper functions

* Remove Functionality that Pulls Env Variables from Empty Keys

* Update api/kv/kv.go

Co-authored-by: Katrina Verey <kn.verey@gmail.com>

* refactor Unmarshal Kustomization struct code

* improve error messages

* Run go mod tidy on all modules before update

* Update sigs.k8s.io/yaml to 1.3.0

* fixed test failure because of latest commits

Signed-off-by: Lars Kellogg-Stedman <lars@oddbit.com>
Co-authored-by: Lars Kellogg-Stedman <lars@oddbit.com>
Co-authored-by: Anna Song <annasong@google.com>
Co-authored-by: yugo kobayashi <kobdotsh@gmail.com>
Co-authored-by: Natasha Sarkar <natashasarkar@google.com>
Co-authored-by: Cailyn Edwards <cailyn.edwards@shopify.com>
Co-authored-by: Cailyn <cailyn.s.e@gmail.com>
Co-authored-by: Katrina Verey <kn.verey@gmail.com>
Co-authored-by: Katrina Verey <katrina.verey@shopify.com>
annasong20 added a commit to annasong20/kustomize that referenced this pull request Jan 12, 2023
* initial changes to rename OrgRepo to RepoPath

* changes to rename Path to KustRootPath

* addressed review comments

* addressed review comments

* docs: Add documentation for namespace transformer

Add a short description of the namespace transformer and example
usage to examples/transformerconfigs/README.md.

References: kubernetes-sigs#629
Signed-off-by: Lars Kellogg-Stedman <lars@oddbit.com>

* Localize patchesJson6902, patchesStrategicMerge, replacements (kubernetes-sigs#4904)

* Localize patchesJson6902, patchesStrategicMerge, replacements

* Address code review feedback

* Improve readability
* Remove deprecation warning check

* Load legacy kustomization fields for `localize` (kubernetes-sigs#4918)

* Load legacy kustomization

* Expose loadKustFile in kusttarget

* remove FixKustomizationPreUnmarshalling

* remove deprecated cfg and fn commands (kubernetes-sigs#4930)

* remove deprecated cfg and fn commands

* fix lint error

* run gofmt

* Localize PatchTransformer, PatchJson6902Transformer (kubernetes-sigs#4920)

* Localize patches, patchesJson6902 custom transformers

* Improve readability

* Localize fields: openapi, configurations, crds (kubernetes-sigs#4907)

* Localize openapi, configurations, crds

* Add integration test

* Move krusty test

* Address code review feedback

* Implement locRootPath (kubernetes-sigs#4909)

* Implement locRootPath, and include userinfo, port in locFilePath

* Strip userinfo, port

* Improve readability

* Localize legacy fields

* Localize resources (kubernetes-sigs#4912)

* Localize resources

* Improve readability

* Add integration tests

* Group test helper functions

* Remove Functionality that Pulls Env Variables from Empty Keys

* Update api/kv/kv.go

Co-authored-by: Katrina Verey <kn.verey@gmail.com>

* refactor Unmarshal Kustomization struct code

* improve error messages

* Run go mod tidy on all modules before update

* Update sigs.k8s.io/yaml to 1.3.0

* fixed test failure because of latest commits

Signed-off-by: Lars Kellogg-Stedman <lars@oddbit.com>
Co-authored-by: Lars Kellogg-Stedman <lars@oddbit.com>
Co-authored-by: Anna Song <annasong@google.com>
Co-authored-by: yugo kobayashi <kobdotsh@gmail.com>
Co-authored-by: Natasha Sarkar <natashasarkar@google.com>
Co-authored-by: Cailyn Edwards <cailyn.edwards@shopify.com>
Co-authored-by: Cailyn <cailyn.s.e@gmail.com>
Co-authored-by: Katrina Verey <kn.verey@gmail.com>
Co-authored-by: Katrina Verey <katrina.verey@shopify.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

4 participants