-
Notifications
You must be signed in to change notification settings - Fork 35
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
Implement Pull Request CI builds #590
Comments
We already had builds for the desktop extension, but my understanding is that it was merged into this repo: |
Correct. |
I've created the multibranch pipeline on Jenkins under |
@markoburcul, tried creating #594 PR to see if the |
Since nix shell function from jenkins librarz uses WORKSPACE env variable to find shell.nix we need to override it for steps using nix shell. For all of the steps I'm using dir directive to change cwd to the apps/connector. Referenced issue: #590 Signed-off-by: markoburcul <[email protected]>
Use newest jenkins lib tag which adds the entryPoint as an argument to the nix shell function. Referenced issue: #590 Signed-off-by: markoburcul <[email protected]>
Use newest jenkins lib tag which adds the entryPoint as an argument to the nix shell function. Referenced issue: #590 Signed-off-by: markoburcul <[email protected]>
* jenkinsfile: fix paths for all steps Use newest jenkins lib tag which adds the entryPoint as an argument to the nix shell function. Referenced issue: #590 Signed-off-by: markoburcul <[email protected]>
The pipeline is successfully up and running. I've enabled |
@markoburcul why didn't this #600 PR get built? |
I've forgot to add webhook for this repo. It is now added and working fine, feel free to test it! |
@markoburcul thank you very much. One more thing, those builds should really just be for that directory and not for #602 (comment), for example. |
Jenkins by default doesn't support filtering of build triggers based on path in the repository where change occured. I've tried on the stage level but it won't work first time the PR is opened which is not acceptable so that option is not possible. Why do you need comments from |
Ideally we would want to trigger Jenkins build based on the path where change occurred. Unfortunately this is not supported by Jenkins. As a workaround we are conditionally executing steps if the change occured in the apps/connector/ path. Prior to this we need to fetch the changelog diff between current branch and target branch to understand if there was a change present in the target dir and if we need to run all stages. The population of changelog is needed just the first time the PR is opened and in the subsequent steps we don't need this. Referenced issue: #590 Signed-off-by: markoburcul <[email protected]>
@markoburcul was about to just reply before you commented and now deleted your message 🙂
Finishing early wouldn't work?
– source, chatgpt
For consistency with other repos like desktop and mobile.
For example. |
Sorry for that, I realized my solution wasn't working as expected!
It would but for other people with opened PR's the check down below would be failed. In the case of no changes I'm affraid the only option is to skip stages after.
Sure. |
You mean those PRs already open and only within this repo – as result of all of this testing? That'd be alright, as long as the new PRs would work as expected (i.e. /apps/connector completes Jenkins jobs and comments while changes to rest don't).
That's what I meant by finishing early and not executing reaming steps from the Jenkins pipeline (i.e. build, zip, comment). |
Here is the PR that does the trick, thanks for the suggestion! |
Thank you @markoburcul |
@markoburcul could you please check what is triggering these builds in #600 (comment) if that PR itself didn't have any new pushes/commits itself? |
The file changed is within the |
Nope. GitHub would show. Something else gotta be triggering those kind of builds, which eventually produces those detached/not searchable commits
https://ci.status.im/job/status-web/job/prs/job/linux/job/PR-600/4/console |
Is this the only one where it happened or there is more? |
PRs? There hasn't been any other for that directory open. Builds? All but the first one. |
I've changed one of the settings for the pipeline. You could test it by opening new pr with a change in apps/connector dir, but you should wait for any new change in the |
Did you 😉? |
I've opened the pr and now I'll wait for any change in the main |
Merged a commit into main. |
But I see your PR wasn't even built #620. |
@felicio as you can see here on this PR which was created after we merged this commit there are no build comments and additionally in Jenkins UI the build wasn't triggered after this commit was pushed to main branch.
|
Created #626 to see if it will build only once and remain so overtime as per the original issue. |
As you can see with this commit we have proven that PR builds won't be triggered by pushes to main. |
Could you check my last comment? I believe that we achieved the desired behavior. |
No new unrelated builds in #600 (comment) and #626 (comment). Closing. Thanks again @markoburcul . |
what
why
how (for example)
The text was updated successfully, but these errors were encountered: