imodel02 PR validation

[pmconne]

Is your feature request related to a problem? Please describe. The imodel02 branch exists for changes to itwinjs-core that depend on changes to the native addon code. Currently PRs against this branch undergo no validation. This leads to problems only being discovered when someone attempts to integrate a newly-published addon into master, often requiring chasing down developers to fix issues or even discarding the newly-published addon if fixes are required in native code. This is a waste of everyone's time and only discourages people from regularly producing new addons, worsening the severity of the problems discovered when doing so.

Describe the solution you'd like Ideally, when a developer submits a PR against the imodel02 branch, the corresponding native changes would be used to produce an addon, and PR validation (build, test, lint, extract-api, docs) would run using that addon, failing the PR if anything goes wrong. Also, required reviewers would be enforced per CODEOWNERS.

Describe alternatives you've considered

Additional context

[calebmshafer]

This can be broken down into two main areas:

• [x] Turn on Codeowner PR validation
• [ ] Add build validation:
  • [ ] Main iTwin.js pipeline
  • [ ] Docs
  • [ ] Integration tests

The codeowner enforcement should be able to happen before agnostic to any build validation running and could be enabled quickly. I will look into that today.

Build Validation

With build validation, there are a few different pieces required since builds need to be coordinated between the two different PRs in different repos. With requiring the @bentley/imodeljs-native changes to be merged into the master branch of that repository at the same time as the Typescript PR is merged into the imodel02 branch, there is an order of operations that needs to happen and a few questions.

Option 1

1. Create @bentley/imodeljs-native PR
2. Two things are happening in parallel a. @bentley/imodeljs-native PR builds and runs all tests. Does not publish packages for testing b. Create itwinjs-core PR into the imodel02 branch with a specific format to "link" to the native PR - A suggested footer on PR description. Native PR: xxxx where xxxx is the PR number but not a link
3. A new iTwin.js pipeline is queued that builds the native portion, publishes as a pipeline artifact and then as a separate stage the iTwin.js build consumes
   • This would include a pipeline needed for each main build & test, Docs and integration tests today

Benefits

• Native side changes are validated prior to the opening the Typescript side

Downsides

• Duplicating the native build, which takes upwards of 60 - 70 minutes in most cases, for every pipeline.

Option 2

1. Create @bentley/imodeljs-native PR
2. @bentley/imodeljs-native PR builds, runs all tests and publishes packages for consumption testing of itwinjs-core.
3. Create itwinjs-core PR into the imodel02 branch with a specific format to "link" to the native PR
   • A suggested footer on PR description. Native PR: xxxx where xxxx is the PR number but not a link
4. A new iTwin.js pipeline is queued that pulls the packages published from step 2

Benefits

• Reduces the overall build machine required by building the native code once and all Typescript side validation consumes it

Downsides

• Increases every native PR time to support publishing packages unless there is something introduced to flag the build to run only if there is a corresponding typescript PR.

Open questions

• How do we handle invalidating the itwinjs-core PR when there is an update on the native side?
• What builds do we need to speed up to make this possible? Analysis is needed on the way we build the native code in order to try and parallelize it more.
• Would we want to validate every native PR by running the itwinjs-core tests? Not sure if this level is needed on every PR but should be done prior to release.
• Do we need build caching for all iTwin.js pipelines to avoid building in each pipeline?

[calebmshafer]

just turned on CodeOwner reviews required.

[pmconne]

Don't we have the option now to eliminate the imodel02 branch and most of the manual addon publishing process entirely?

[calebmshafer]

Don't we have the option now to eliminate the imodel02 branch and most of the manual addon publishing process entirely?

@pmconne not quite sure what you mean. Would you want to publish a new addon version for every native change?

[pmconne]

Would you want to publish a new addon version for every native change?

The fork (and soon, repo) for the native code exists solely for changes we want to incorporate into the addon. Why shouldn't each be a discrete version instead of someone else manually publishing accumulated changes into a new version at some later date? Your option 2 suggests publishing native packages for every native PR anyway. Some native PRs will have no corresponding itwinjs-core changes but can nevertheless break and therefore be validated against itwinjs-core.

If every change to the addon maps to a PR commit on master that makes a linear history that we're currently lacking. That lack often makes it very difficult to track down in which commits a regression occurred. @DStradley and I are dealing with such a situation right now, where a huge number of imodel02 commits were forward-merged into master as part of addon PR and we need to dig around to find which of those broke something. And checking out most of those intermediate commits produces a source tree that does not build, because the commit relied on an intermediate state of the native code and/or contained errors that were only corrected when integrating the new addon. Every commit to master should be independently buildable.

[skirby1996]

If we are using a pipeline triggered by a native PR to drive this process, and linking a typescript PR to that native PR build, we can post commit statuses back up to the linked typescript PR, and update that status manually as the build runs. You can see an example of what those statuses look like in a PR here. You can either create new statuses, or update existing ones using this API.

One point to note, is that a commit status targets a specific commit rather than a PR or a branch. If a new commit is pushed up to a typescript PR, it will only list those statuses associated with the HEAD commit of the PR branch. However we pull in the SHA for the head commit of the linked typescript PR in our pipeline, we would have to rerun the larger PR anytime a new commit was pushed up on the typescript side.

As far as requiring these commit status in a PR, the simplest approach would be to require that check as one of the branch protection conditions for the "imodel02" branch in GH. If more flexibility is required, we can use the post_check action provided by mergify, and define a more complex series of checks/conditions. We could then require that check as a branch protection rule instead.

[pmconne]

@calebmshafer @skirby1996 what's the status of this? I'm tired of wasting time fixing everybody's merge conflicts, tests, lint, etc etc. Meanwhile perhaps we need a rule that after you merge a PR to OpenSourceFork you are responsible to publish + integrate new addon.

[DanRod1999]

Still working on the pipeline. Close to completion but hit an issue and currently looking for solution.

[pmconne]

@DanRod1999 any progress?

[DanRod1999]

@DanRod1999 any progress?

I put in two draft PR's, but haven't been able to do look over them with anyone. My pipeline appears to function properly except when rush cover runs on Linux. Also I had to change a few pipelines that used a template that I changed, so I need to make sure those aren't broken

https://github.com/iTwin/itwinjs-core/pull/4278 https://dev.azure.com/bentleycs/iModelTechnologies/_git/imodel02/pullrequest/278935 https://dev.azure.com/bentleycs/iModelTechnologies/_build/results?buildId=1680461&view=logs&j=b002de06-6b71-5d1f-d201-8e3d22ebcd72&t=6b47f363-a71a-5215-ba5b-263e7c85bc93

[pmconne]

Status? Are you just still waiting for someone to look at your PRs? @calebmshafer I am wasting so much time trying to track down image regressions.

[DanRod1999]

Status? Are you just still waiting for someone to look at your PRs? @calebmshafer I am wasting so much time trying to track down image regressions.

I ran into some issues earlier this week, but now its fixed. I was going to send out an email to Caleb, Bill, and Seamus to look over the PRs now. The only problem I'm still running into is rush cover always fails on Ubuntu-Latest vm, and I can't run that job on our imodeljs linux agents because they don't have 'unzip' installed. I need to talk to @calebmshafer to see if we can install 'unzip' to fix that problem, since imodeljs linux is the appropriate pool for this anyway.

Other than that the pipeline I believe is done, and I wanted to talk to @skirby1996 about testing it, but I believe he was out earlier this week

[aruniverse]

The only problem I'm still running into is rush cover always fails on Ubuntu-Latest vm, and I can't run that job on our imodeljs linux agents because they don't have 'unzip' installed. I need to talk to @calebmshafer to see if we can install 'unzip' to fix that problem, since imodeljs linux is the appropriate pool for this anyway.

Talk to Chuck K

[calebmshafer]

@DanRod1999 for now just do an apt install for the package you need as a script task in the build.

We can update the actual image later. I can show you where to make that change tomorrow.

[DanRod1999]

Validation between core and native https://github.com/iTwin/imodel-native/pull/56 https://github.com/iTwin/imodel-native-internal/pull/22

Validation between native and internal https://github.com/iTwin/imodel-native-internal/pull/38