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

Add document for using task migrations #1862

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tkdchen
Copy link
Contributor

@tkdchen tkdchen commented Jan 27, 2025

@tkdchen tkdchen requested a review from a team as a code owner January 27, 2025 09:12
Historically, task maintainers write `MIGRATION.md` to notify users what changes
have to be made to the pipeline. This mechanism is not deprecated. Besides
writing the file, it is also recommended to write a migration so that the
updates can be applied to user pipelines automatically.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would also be good to mention how the migration files get applied - i.e. link to the migration tool and the configuration that enables it in our Renovate deployment (Mintmaker)

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 sounds like a developer guide. How about write it down after the improvement is done? What do you think we start a new document separately for that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking we can just add a link to the migration tool, e.g.

...so that the updates can be applied to user pipelines automatically (by the pipeline-migration-tool)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Comment on lines +260 to +261
(.spec.tasks[] | select(.name == "task-a") | .params) +=
{"name": "pipelinerun-name", "value": "\$(context.pipelineRun.name)"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make this example idempotent? (I.e. only add the param if it's not already there)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's already idempotent. Line 257 checks that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, I missed that. Maybe add a short comment about that above the if? It's not immediately obvious that that's why the check is there

@chmeliik
Copy link
Contributor

We should probably keep this open until the migration tool is ready for use?

@tkdchen tkdchen requested a review from chmeliik February 10, 2025 06:24
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.

2 participants