Add lint step to version bump script
Closes #8270: Automatic deprecation date linting on release
Added logic to version-bump.yaml script:
- Run ESLint rule on release branch to modify
@deprecatedcomments in code. Comments are modified by adding deprecation dates. Those dates mark when a specific piece of code may be removed with a major release. Dates are added to@deprecatedcomments if no date is yet specified OR an already passed date is changed with a text saying that code may be removed in next major version. - Those same changes are attempted to be cherry-picked onto
masteras well. - If a conflict happens when cherry-picking changes onto master, then those changes (with conflicts) are pushed to a separate branch and a PR is created. It is then the job of a developer to fix conflicts on that branch and merge PR into
master.
Bellow I will provide the results of me testing these changes. I tested out these new steps on a fork of itwinjs-core.
Test 1
Conditions: BumpType set as patch. ESLint rule does make deprecation commend changes on release branch but no conflicts happen when cherry-picking those changes onto master.
Azure pipeline run: https://dev.azure.com/bentleycs/iModelTechnologies/_build/results?buildId=4069642&view=logs&j=e1047b3d-01b1-51fb-6d3a-7a524f324c33&t=25195add-2513-5480-ce76-76a245f3127b
Commits made to master during version bump process:
Commits made to release branch during version bump process:
"Create release" and "finalize release" GitHub actions are still being triggered successfully:
- https://github.com/augustasgri/itwinjs-core-fork/actions/runs/15921649419/job/44909593896
- https://github.com/augustasgri/itwinjs-core-fork/actions/runs/15921649575/job/44909594267
A release is still successfully being created: https://github.com/augustasgri/itwinjs-core-fork/releases/tag/release%2F5.14.7
Test 2
Test 3
@DanRod1999 plz review
in test three what caused the cherry pick to have conflicts. Also who is notified when conflicts happened and a PR is opened? Is someone auto assigned to the PR?
Changes look fine, and you're tests look good. My only concern is that this is a pretty hefty addition to this pipeline. Does it seem rational to make this its own pipeline? And instead of a template that is called by this pipeline, we create a new azure pipeline (or gh workflow, which ever is easier/more appropriate) and then trigger this new pipeline from this version bump script. Might look something like this:
az pipelines run \
--branch $(Build.SourceBranch) \ // Not sure what branch is appropriate here, but I assume source branch would work
--id 8467 \ // This id or name would be whatever we create it as
--parameters "BumpType={{ parameter.bumpType }}" ... ect
Some advantages of adding this as its own pipeline are:
- should be easier to debug any problems if something goes wrong since we don't need to look into the version bump script
- can run pipeline job independently from original bump pipeline script if something goes wrong (might not be useful for this scripts intent?)
My only concern is that this is a pretty hefty addition to this pipeline. Does it seem rational to make this its own pipeline?
@DanRod1999 I think our release process is already hard to follow and there are too many pipelines. It's confusing when one pipeline run triggers another, ideally I would just have one. We can have different pipelines for different use cases, though (like different release types).
On the other hand, while this script is new and until we build some trust around it, maybe we could add a manual validation step before actually pushing this to npm (unless it's already there). Of course, the validation would be skipped if the script made no changes.
maybe we could add a manual validation step before actually pushing this to npm (unless it's already there). Of course, the validation would be skipped if the script made no changes.
this will run before it is published to npm. the workflow is
- bump version (the pipeline edited in this PR)
- build code
- publish to npm (this are all separate pipelines, which as you mentioned, does make things a bit confusing)
in test three what caused the cherry pick to have conflicts.
The conflicts were introduced manually. I had to modify either the master or release branch before running the script to simulate a cherry-pick failure.
In theory, these conflicts shouldn't occur, since developers shouldn't need to edit existing deprecated comments. However, in practice, it's likely that someone might unintentionally or intentionally edit these comments, for example by moving code around. I'm not sure how well cherry-pick handles such cases automatically, so we decided to automate the conflict resolution process as much as possible.
who is notified when conflicts happened and a PR is opened?
After discussing it with Paulius, we decided to tag Arun in PR description. The PR notification will be sent, but it might end up in spam. At least there is going to be some record of the PR. But we expect the developer who is running this pipeline to notice the conflicts, fix them and merge the PR. I know this expectation is currently not very well documented and mainly spread by word. If you have any ideas of how to inform people of this let me know.
On the other hand, while this script is new and until we build some trust around it, maybe we could add a manual validation step before actually pushing this to npm (unless it's already there). Of course, the validation would be skipped if the script made no changes.
Added a manual validation step to fast-ci.yaml pipeline. This step triggers only if eslint command made changes during version bump process. The validation step times out after 3 days. Once this step is triggered Arun is notified (however, the developer running the pipeline should notice this job and approve themselves).
On the other hand, while this script is new and until we build some trust around it, maybe we could add a manual validation step before actually pushing this to npm (unless it's already there). Of course, the validation would be skipped if the script made no changes.
Added a manual validation step to
fast-ci.yamlpipeline. This step triggers only if eslint command made changes during version bump process. The validation step times out after 3 days. Once this step is triggered Arun is notified (however, the developer running the pipeline should notice this job and approve themselves).
@aruniverse ^ (see both comments above)
On the other hand, while this script is new and until we build some trust around it, maybe we could add a manual validation step before actually pushing this to npm (unless it's already there). Of course, the validation would be skipped if the script made no changes.
Added a manual validation step to
fast-ci.yamlpipeline. This step triggers only if eslint command made changes during version bump process. The validation step times out after 3 days. Once this step is triggered Arun is notified (however, the developer running the pipeline should notice this job and approve themselves).@aruniverse ^ (see both comments above)
Im flattered, but I shouldnt the bottleneck here... Can we assign @iTwin/itwinjs-core-admins to those PRs?
This pull request is now in conflicts. Could you fix it @augustasgri? 🙏 To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/