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

Revert OCI and Location #2814

Merged
merged 2 commits into from
Feb 18, 2022
Merged

Revert OCI and Location #2814

merged 2 commits into from
Feb 18, 2022

Conversation

martinmaly
Copy link
Contributor

Im preparation to merge porch branch into main:

Preparing for merge of porch branch back into main,
Preparing for merge of porch branch back into main.
Copy link
Contributor

@droot droot left a comment

Choose a reason for hiding this comment

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

@loudej Can you also take a look at it. Thanks

Copy link
Contributor

@loudej loudej left a comment

Choose a reason for hiding this comment

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

LGTM - the oci-support was intended to be a "light touch" change and these diffs all look like the places where existing code was adjusted when it was git-specific (e.g. where it was coded directly against kptfile.upsteam.git structs)

return nil
}

// cloneAndCopy fetches the provided repo and copies the content into the
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note - this would be a good file to bookmark - that in a PR to main if there are no diffs or if there are that the main is authoritative.

Unless the text of the code came from the current main (rather than a "commit revert") - then having no diffs should be almost certain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. I will still need to be very careful with the merge into main.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirmed. this file is near identical; only diff relative to main are filesystem changes.

kptfilev1.Git
Destination string
}

func GitParseArgs(ctx context.Context, args []string) (GitTarget, error) {
g := GitTarget{}
func GitParseArgs(ctx context.Context, args []string) (Target, error) {
Copy link
Contributor

@loudej loudej Feb 18, 2022

Choose a reason for hiding this comment

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

Same note - this file is also good to spot check. Actually - rather than wait for PR to main - you could spot-check now with a copy-paste of the entire raw file from main in your working copy to verify that it remains "unchanged".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack. Will double check this file too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this one is identical to main


// shouldUpdateSubPkgRef checks if subpkg ref should be updated.
// This is true if pkg has the same upstream repo, upstream directory is within or equal to root pkg directory and original root pkg ref matches the subpkg ref.
func shouldUpdateSubPkgRef(subKf, rootKf *kptfilev1.KptFile, originalRootKfRef string) bool {
Copy link
Contributor

@loudej loudej Feb 18, 2022

Choose a reason for hiding this comment

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

Point of interest --- this is exactly where the location.Rel comes from. The need to know that two remote locations are identical in repo and revision - and the if one's path is under or equal to the other - except w/out being tightly coupled to understanding the remote location types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks!

// read KptFile cloned with the package if it exists
kptfile, err := pkg.ReadKptfile(filesys.FileSystemOrOnDisk{}, path)
kpgfile, err := pkg.ReadKptfile(filesys.FileSystemOrOnDisk{}, path)
Copy link
Contributor

Choose a reason for hiding this comment

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

spin off --- keeping this PR a pure revert is a good idea, but there's a minor local variable typo here that would be nice to fix in main

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah :) I actually fixed this typo and much of my battling with rebases last night was trying to keep this typo fixed; ultimately I gave up; will fix in main.

@martinmaly martinmaly merged commit 2307648 into kptdev:porch Feb 18, 2022
@martinmaly martinmaly deleted the revert branch February 22, 2022 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants