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

Merge pipeline design doc #2623

Merged
merged 7 commits into from
Jan 14, 2022
Merged

Conversation

phanimarupaka
Copy link
Contributor

This doc introduces a new feature to merge the pipeline section in the Kptfile. This has been a problem since the release of kpt v1 and this doc hopes to solve that.

docs/design-docs/03-pipeline-merge.md Outdated Show resolved Hide resolved
docs/design-docs/03-pipeline-merge.md Outdated Show resolved Hide resolved
docs/design-docs/03-pipeline-merge.md Outdated Show resolved Hide resolved
docs/design-docs/03-pipeline-merge.md Outdated Show resolved Hide resolved
docs/design-docs/03-pipeline-merge.md Show resolved Hide resolved
docs/design-docs/03-pipeline-merge.md Outdated Show resolved Hide resolved
@phanimarupaka phanimarupaka mentioned this pull request Dec 19, 2021
@phanimarupaka
Copy link
Contributor Author

cc @morgante @jafcrocker Please review the doc and provide feedback.

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.

This is looking good to me. Nailing down the identify of the items in the pipeline definitely makes the problem more tractable.

docs/design-docs/03-pipeline-merge.md Outdated Show resolved Hide resolved
docs/design-docs/03-pipeline-merge.md Show resolved Hide resolved
docs/design-docs/03-pipeline-merge.md Outdated Show resolved Hide resolved
docs/design-docs/03-pipeline-merge.md Outdated Show resolved Hide resolved
docs/design-docs/03-pipeline-merge.md Outdated Show resolved Hide resolved
docs/design-docs/03-pipeline-merge.md Outdated Show resolved Hide resolved
docs/design-docs/03-pipeline-merge.md Outdated Show resolved Hide resolved
@mengqiy
Copy link
Contributor

mengqiy commented Dec 24, 2021

From what I learned when working on Strategic Merge Patch:

  • Most users don't want to learn complex merge semantics.
  • The more intuitive the semantics is, the better.

There are 3 critical parts we need to design here:

  • the merge key(s)
  • the behavior when adding, updating and removing entries.
  • ordering

IMO we should simplify the merge key design. I'd suggest using name if it presents. Otherwise use image. If there are multiple entries with the same image but no name field, we keep doing what we do today to be backward compatible but we print a warning message like name field is required to distinguish entries with same image name when merging the pipeline. We don't use other fields.

Relative order in the updated upstream and local should be preserved. (See #2623 (comment)). If the order conflict, we should define what strategy or strategies we will use. e.g. upstream-win and local-win

@phanimarupaka
Copy link
Contributor Author

@mengqiy @droot I agree that the description should be simple. But just going by the image name will lead to deletion of functions if multiple functions with same image are declared(e.g. search-replace). We need not document all the nuances and just mention that only image is used as fall back merge key in case name is not specified. Users can leverage name field if they have multiple functions with same image to have deterministic merge behavior. But behind the scenes we will make best effort to not delete the functions with this approach.

@mortent
Copy link
Contributor

mortent commented Jan 5, 2022

IMO we should simplify the merge key design. I'd suggest using name if it presents. Otherwise use image. If there are multiple entries with the same image but no name field, we keep doing what we do today to be backward compatible but we print a warning message like name field is required to distinguish entries with same image name when merging the pipeline. We don't use other fields.

This is mostly the same approach I suggested in an earlier comment. This leaves three different ways to do the merge (name merge key, image merge key and non-associative list). I'm not convinced that using the image merge key approach is necessary, but this seems to be pretty easy to explain.

@phanimarupaka
Copy link
Contributor Author

@mortent I agree. We should use image as the fall back option for merge key. And users should only be exposed to that detail in the user facing docs. What if users are not aware of this and declared multiple functions with same image and no name field specified ? I am doing the best effort here to not delete the functions during merge. This detail need not be exposed to the user. We can just say, the behavior will not be deterministic and use name field.

docs/design-docs/03-pipeline-merge.md Outdated Show resolved Hide resolved
docs/design-docs/03-pipeline-merge.md Show resolved Hide resolved
docs/design-docs/03-pipeline-merge.md Show resolved Hide resolved
docs/design-docs/03-pipeline-merge.md Show resolved Hide resolved
docs/design-docs/03-pipeline-merge.md Show resolved Hide resolved
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.

I am happy with the result on this, so it's lgtm from me. Let's have @mortent and @mengqiy take a final look.

@phanimarupaka phanimarupaka merged commit caa8c6a into kptdev:main Jan 14, 2022
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.

4 participants