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

Public Linking PR Docs For #906

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

Public Linking PR Docs For #906

wants to merge 4 commits into from

Conversation

DanRod1999
Copy link
Contributor

Public Docs for anyone who might use PR validation. Also greatly slimmed down from current docs, and hopefully easier to read.

Don't know if location is great. Maybe it should be in a Docs dir or something?

@DanRod1999
Copy link
Contributor Author

/azp run imodel-native

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DanRod1999 DanRod1999 changed the title Dan/linking-pr-docs Public Linking PR Docs For Nov 13, 2024
@aruniverse aruniverse requested review from nick4598 and removed request for chuckkir January 10, 2025 13:48
build/linking-prs.md Outdated Show resolved Hide resolved

## Expected Behavior
### For a native and core change
1. The native pipeline should begin and build to completion. The `Build itwinjs-core` Stage should not be skipped
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the Build itwinjs-core stage skipped ever? 'should not be skipped' sort of implies to me that it might be in some cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically they can be skipped, but it depends on specifics. There's a long list of conditions for that stage. It generally shouldn't, but i'm not sure if its 100%

### For a native and core change
1. The native pipeline should begin and build to completion. The `Build itwinjs-core` Stage should not be skipped
1. There is currently no solution for stopping the iTwinJs Pipeline, so that will run as if nothing is linked, and will most likely fail.
1. Upon completion of the native pipeline, the status check on your core PR should be updated
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the status check have a name?

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 does, but I think it might not be necessary anymore. This was more important before addons were automatically integrated. I think this can be removed now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when a new addon is pushed to the branch it deletes that check anyway, so its no longer visible

build/linking-prs.md Outdated Show resolved Hide resolved
- `imodel-native: https://github.com/iTwin/imodel-native/pull/PULL-NUM`
1. If you already created the PRs just add this links to your PR description and then in the imodel-native PR comment `/azp run imodel-native` to retrigger the pipelines.
1. Wait until your pipeline succeeds and *both* PRs are approved.
1. One both PRs are approved merge the imodel-native PR. This will create a new release with your change and automatically add the new release of the addon to your itwinjs-core PR. (This may take a few hours to build and publish your changes)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess since these are 'public' docs that means the intended audience will not have access to the pipelines that are running right? Theres sort of a hidden extra step here that you should be watching over the pipeline that is building your code to create a new release, right? Does that mean bentley developers will be expected to perform that step for them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, tbh devs don't do that now anyway, but people should be watching for the addons to be created. This is party why i don't know if its worth even having a document like this. Should we even allow someone outside to attempt a change like this. Is it even realistic to expect someone to be able to make a change across repos without the ability to build native (and maybe core, because certain tests i believe require permissions)

Copy link
Contributor

@nick4598 nick4598 Jan 10, 2025

Choose a reason for hiding this comment

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

Its a good point, does feel like we are skipping the part where devs should be able to build native locally and jumping straight to letting them know how to make a PR. Maybe simpler PRs would be possible , but can they even see build errors in our private pipeline runs to iterate on their code? ( I dont think so?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe within the gh screen, but not sure tbh. I know GH shows some kind of error message from azure, but I never really look closely to see how useful it is

DanRod1999 and others added 2 commits January 10, 2025 13:06
Co-authored-by: Nick Tessier <[email protected]>
Co-authored-by: Nick Tessier <[email protected]>
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