feat: localize absolute paths #5294

typeid · 2023-08-25T16:16:56Z

Attempts to fix #5275.

This PR adds handling of absolute paths to kustomize localize. To enable this, it converts the absolute paths to relative paths, keeping the rest of the processing chain intact.

Please let me know if anything's missing (e.g. documentation), or if this is going in the wrong direction. It's my first contribution here :)

k8s-ci-robot · 2023-08-25T16:17:03Z

Welcome @typeid!

It looks like this is your first PR to kubernetes-sigs/kustomize 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kustomize has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2023-08-25T16:17:04Z

Hi @typeid. 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 · 2023-09-15T18:57:31Z

@rayandas Could you give this PR a review?

rayandas · 2023-09-15T19:25:16Z

@natasha41575 Sure.
/assign

api/loader/fileloader.go

api/krusty/accumulation_test.go

api/loader/fileloader_test.go

stormqueen1990

Hi there, @typeid!

Left some comments here. It seems this branch also needs to be rebased.

api/internal/localizer/locloader.go

api/loader/fileloader_test.go

stormqueen1990

/lgtm

ezzoueidi · 2023-10-09T20:06:07Z

Sorry for the delay.

/lgtm for me as well. Thank you!

natasha41575 · 2023-10-10T18:29:55Z

Thanks for your contribution! We'll try to get an approver to look at this in the next week or so.

annasong20

@typeid Thank you for contributing to localize! I think the core logic is there, we just need to scope down the PR and add tests. I can re-review after you've addressed the below comments.

api/internal/loader/fileloader.go

api/internal/loader/fileloader_test.go

api/krusty/accumulation_test.go

annasong20 · 2023-10-13T20:35:58Z

api/internal/localizer/locloader.go

 	if filepath.IsAbs(path) {
-		return nil, errors.Errorf("absolute paths not yet supported in alpha: file path %q is absolute", path)
+		if !strings.HasPrefix(path, ll.Root()) {
+			return nil, fmt.Errorf("referenced absolute path '%s' in your Kustomization file is not in the same tree as '%s'", path, ll.Root())
+		}
+
+		// Convert absolute path to relative path for unified handling
+		path, err = filepath.Rel(ll.Root(), path)
+		if err != nil {
+			return nil, fmt.Errorf("could not convert absolute path '%s' to relative path of root", path)
+		}
 	}


We only need to remove the previous check and ensuing error return.

Do we need this extra code segment? From what I see of your change in cleanFilePath, you already handle passing in an absolute path, so we don't need to find the relative path in this code segment? Please correct me where I err.

You're correct. I removed this redundant segment.

I had initially added the handling for absolute paths in Load only, but cleanFilePath is also called outside of it, so it broke as it couldn't handle absolute paths. Then I ended up handling it twice instead of centralizing it into just cleanFilePath.

Also, I now see that the sanity checking of whether a file is inside of the root is already done in FileLoader.Load(), which made most of previous my implementation redundant.

api/internal/localizer/locloader_test.go

api/internal/localizer/util.go

annasong20 · 2023-10-13T23:21:25Z

api/internal/localizer/util.go

@@ -114,7 +114,11 @@ func hasRef(repoURL string) bool {

 // cleanFilePath returns file cleaned, where file is a relative path to root on fSys
 func cleanFilePath(fSys filesys.FileSystem, root filesys.ConfirmedDir, file string) string {


This PR currently writes a localized relative path in-place of the original absolute path in the localized copy. I'm down to keep this behavior, as opposed to preserving the absolute vs. relative nature of file paths, but can we upload a documentation PR to https://github.com/kubernetes-sigs/cli-experimental describing this behavior? We can also announce there the ability to localize absolute paths! We won't merge it until this one goes in.

Makes sense, I'll look into that repo, open a PR and link it here once it's ready.

kubernetes-sigs/cli-experimental#367 Let me know if this is enough or if it needs a bigger mention. I wasn't sure how to "version" the mention of the alpha command not handling absolute paths.

api/internal/localizer/util.go

k8s-ci-robot · 2023-11-26T11:15:50Z

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.

k8s-triage-robot · 2024-02-24T12:23:25Z

The Kubernetes project currently lacks enough contributors to adequately respond to all 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:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

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

/lifecycle stale

stormqueen1990 · 2024-02-24T13:34:38Z

/remove-lifecycle stale /auto-cc

stormqueen1990

Thank you for your contribution, @typeid! 😃

/lgtm
/cc @varshaprasad96
for maintainer review

varshaprasad96

The changes look good to me. Thanks for the contribution @typeid!
/lgtm
/approve

k8s-ci-robot · 2024-04-30T04:59:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: typeid, varshaprasad96

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

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 Aug 25, 2023

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 25, 2023

k8s-ci-robot requested review from natasha41575 and yuwenma August 25, 2023 16:17

typeid force-pushed the localize_absolute_paths branch 4 times, most recently from 5c3befe to 8762415 Compare August 25, 2023 16:45

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 Aug 28, 2023

k8s-ci-robot assigned rayandas Sep 15, 2023

ezzoueidi reviewed Sep 20, 2023

View reviewed changes

api/loader/fileloader.go Outdated Show resolved Hide resolved

api/krusty/accumulation_test.go Outdated Show resolved Hide resolved

api/loader/fileloader_test.go Outdated Show resolved Hide resolved

typeid force-pushed the localize_absolute_paths branch from 8762415 to e3e4f87 Compare September 20, 2023 16:34

natasha41575 assigned ezzoueidi Sep 20, 2023

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

natasha41575 assigned annasong20 and unassigned ezzoueidi and rayandas Oct 6, 2023

stormqueen1990 reviewed Oct 7, 2023

View reviewed changes

api/internal/localizer/locloader.go Outdated Show resolved Hide resolved

api/loader/fileloader_test.go Outdated Show resolved Hide resolved

typeid force-pushed the localize_absolute_paths branch from e3e4f87 to 3ec212e Compare October 8, 2023 09:26

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 8, 2023

stormqueen1990 reviewed Oct 9, 2023

View reviewed changes

k8s-ci-robot assigned stormqueen1990 Oct 9, 2023

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

annasong20 suggested changes Oct 14, 2023

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 26, 2023

typeid force-pushed the localize_absolute_paths branch from 6e7113c to 141a7ea Compare November 26, 2023 11:18

feat: localize absolute paths

7b1eaf1

typeid force-pushed the localize_absolute_paths branch from 141a7ea to 7b1eaf1 Compare November 26, 2023 11:22

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 26, 2023

typeid mentioned this pull request Nov 26, 2023

Update documentation for localizing absolute paths kubernetes-sigs/cli-experimental#367

Closed

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

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 24, 2024

k8s-ci-robot requested review from stormqueen1990 and varshaprasad96 February 24, 2024 13:35

stormqueen1990 reviewed Mar 24, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 24, 2024

varshaprasad96 approved these changes Apr 30, 2024

View reviewed changes

k8s-ci-robot assigned varshaprasad96 Apr 30, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 30, 2024

k8s-ci-robot merged commit 49a645f into kubernetes-sigs:master Apr 30, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: localize absolute paths #5294

feat: localize absolute paths #5294

typeid commented Aug 25, 2023

k8s-ci-robot commented Aug 25, 2023

k8s-ci-robot commented Aug 25, 2023

natasha41575 commented Sep 15, 2023

rayandas commented Sep 15, 2023

stormqueen1990 left a comment

stormqueen1990 left a comment

ezzoueidi commented Oct 9, 2023

natasha41575 commented Oct 10, 2023

annasong20 left a comment

annasong20 Oct 13, 2023

typeid Nov 26, 2023

annasong20 Oct 13, 2023

typeid Nov 26, 2023

typeid Nov 26, 2023

k8s-ci-robot commented Nov 26, 2023

k8s-triage-robot commented Feb 24, 2024

stormqueen1990 commented Feb 24, 2024 via email

stormqueen1990 left a comment

varshaprasad96 left a comment

k8s-ci-robot commented Apr 30, 2024

		@@ -114,7 +114,11 @@ func hasRef(repoURL string) bool {

		// cleanFilePath returns file cleaned, where file is a relative path to root on fSys
		func cleanFilePath(fSys filesys.FileSystem, root filesys.ConfirmedDir, file string) string {

feat: localize absolute paths #5294

feat: localize absolute paths #5294

Conversation

typeid commented Aug 25, 2023

k8s-ci-robot commented Aug 25, 2023

k8s-ci-robot commented Aug 25, 2023

natasha41575 commented Sep 15, 2023

rayandas commented Sep 15, 2023

stormqueen1990 left a comment

Choose a reason for hiding this comment

stormqueen1990 left a comment

Choose a reason for hiding this comment

ezzoueidi commented Oct 9, 2023

natasha41575 commented Oct 10, 2023

annasong20 left a comment

Choose a reason for hiding this comment

annasong20 Oct 13, 2023

Choose a reason for hiding this comment

typeid Nov 26, 2023

Choose a reason for hiding this comment

annasong20 Oct 13, 2023

Choose a reason for hiding this comment

typeid Nov 26, 2023

Choose a reason for hiding this comment

typeid Nov 26, 2023

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 26, 2023

k8s-triage-robot commented Feb 24, 2024

stormqueen1990 commented Feb 24, 2024 via email

stormqueen1990 left a comment

Choose a reason for hiding this comment

varshaprasad96 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 30, 2024