itwinjs-core icon indicating copy to clipboard operation
itwinjs-core copied to clipboard

Add lint step to version bump script

Open augustasgri opened this issue 6 months ago • 1 comments

Closes #8270: Automatic deprecation date linting on release

Added logic to version-bump.yaml script:

  1. Run ESLint rule on release branch to modify @deprecated comments 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 @deprecated comments 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.
  2. Those same changes are attempted to be cherry-picked onto master as well.
  3. 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: image

Commits made to release branch during version bump process: image

"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

augustasgri avatar Jun 25 '25 11:06 augustasgri

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Jun 25 '25 11:06 CLAassistant

@DanRod1999 plz review

aruniverse avatar Jun 30 '25 15:06 aruniverse

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?

DanRod1999 avatar Jun 30 '25 15:06 DanRod1999

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?)

DanRod1999 avatar Jun 30 '25 15:06 DanRod1999

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.

paulius-valiunas avatar Jun 30 '25 16:06 paulius-valiunas

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)

DanRod1999 avatar Jun 30 '25 16:06 DanRod1999

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.

augustasgri avatar Jul 09 '25 10:07 augustasgri

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).

augustasgri avatar Jul 09 '25 10:07 augustasgri

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).

@aruniverse ^ (see both comments above)

paulius-valiunas avatar Jul 10 '25 12:07 paulius-valiunas

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).

@aruniverse ^ (see both comments above)

Im flattered, but I shouldnt the bottleneck here... Can we assign @iTwin/itwinjs-core-admins to those PRs?

aruniverse avatar Jul 14 '25 19:07 aruniverse

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/

mergify[bot] avatar Jul 17 '25 15:07 mergify[bot]