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

On applying a patch, patch conflicts should allow for interactive resolution #76

Closed
aiyengar2 opened this issue Jan 28, 2022 · 3 comments
Assignees
Labels
enhancement New feature or request team/area3

Comments

@aiyengar2
Copy link
Contributor

aiyengar2 commented Jan 28, 2022

On running a make prepare today, if applying a patch fails we just error out.

Instead, running make prepare should be an interactive process where, if a three way patch between the upstream, scripts, and the user is observed, the user should have the ability to remediate the errors before continuing a the call, similar to the behavior exhibited by a git rebase on encountering conflicts.

make charts, which uses make prepare under the hood, should also specify a non-interactive call of make prepare, which will fail out if it encounters any patches.

@aiyengar2 aiyengar2 self-assigned this Jan 28, 2022
@aiyengar2 aiyengar2 added the enhancement New feature or request label Jan 28, 2022
@aiyengar2 aiyengar2 changed the title make patch should collect patch conflicts and allow for patch resolution instead of exiting On applying a patch, patch conflicts should allow for resolution Jan 28, 2022
@aiyengar2
Copy link
Contributor Author

By adding this feature, rebasing (e.g. changing the upstream of a chart from something like kube-prometheus-stack x.y.z to kube-prometheus-stack (x+1).0.0) would become far easier, since you could just change the upstream and run make prepare, resolve any patch conflicts, and you are good to go.

@aiyengar2
Copy link
Contributor Author

Resolving this issue will help resolve #31, since rebasing would be natively supported in the scripts with this change.

@aiyengar2 aiyengar2 added this to the 0.4.x milestone Jan 28, 2022
@aiyengar2 aiyengar2 changed the title On applying a patch, patch conflicts should allow for resolution On applying a patch, patch conflicts should allow for interactive resolution Jan 28, 2022
@aiyengar2
Copy link
Contributor Author

The fundamental blocker for this issue is that we currently do not store the original base that was used to produce the patch.

As a result, when reading information from a .diff file, the line numbers that look like @@ -5,6 +5,19 @@ (which normally provides context of where to apply a patch; -5,6 says that the original file was modified in the first 6 lines after line 5 and +5,19 says the new file will have 19 lines instead of the original 6 and still start at line 5) have no meaning since we cannot identify what -5,6 corresponds to on the new patch base.

Why does this change if we have the original base?

If we had the original base used to produce the patch, we could diff the new base from the old base.

This would allow us to identify all single-line modifications made (e.g. adds or removals) from old->new

Via the original patch file, we also have all single-line modifications made from old->patched.

Therefore, we can perform a Weave Merge between the two sets of single-line modifications to produce the correct conflict file.


As a result, I'm closing out this issue in favor of #31 since the only two ways I can think to resolve this are:

  1. store some sort of packages/PACKAGE/patch-base directory that stores the equivalent of charts-original in the git repository -> bad idea, since we don't want more files than we already have
  2. support this via a special rebase command that would use the existing package.yaml as the way to derive the original base and take in a rebase.yaml (or set of command line provided answers to questions) that would point at the new base
  • note: this would not resolve the issue where a package.yaml was directly updated or resolve the issue where the package.yaml points to a modified upstream (e.g. upstream changes the base chart archive), since in both cases we would not be able to retrieve the original base that the patch was applied on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request team/area3
Projects
None yet
Development

No branches or pull requests

2 participants