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

Implement Pull Request CI builds #590

Closed
felicio opened this issue Oct 3, 2024 · 31 comments
Closed

Implement Pull Request CI builds #590

felicio opened this issue Oct 3, 2024 · 31 comments
Assignees

Comments

@felicio
Copy link
Collaborator

felicio commented Oct 3, 2024

what

why

  • Allow other teams to test desktop integration independently

how (for example)

@jakubgs
Copy link
Member

jakubgs commented Oct 3, 2024

We already had builds for the desktop extension, but my understanding is that it was merged into this repo:
https://ci.status.im/job/status-browser-extension/

@felicio
Copy link
Collaborator Author

felicio commented Oct 3, 2024

We already had builds for the desktop extension, but my understanding is that it was merged into this repo: https://ci.status.im/job/status-browser-extension/

Correct.

@markoburcul
Copy link
Contributor

I've created the multibranch pipeline on Jenkins under status-web/prs/linux: https://ci.status.im/job/status-web/job/prs/job/linux/ ,
if it is possible could someone from your team test it @felicio ?

@felicio
Copy link
Collaborator Author

felicio commented Oct 10, 2024

@markoburcul, tried creating #594 PR to see if the status-im-auto's comment with builds would show up for changes in https://github.com/status-im/status-web/tree/main/apps/connector directory. So far nothing.

markoburcul added a commit that referenced this issue Oct 10, 2024
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]>
markoburcul added a commit that referenced this issue Oct 10, 2024
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]>
markoburcul added a commit that referenced this issue Oct 10, 2024
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]>
markoburcul added a commit that referenced this issue Oct 10, 2024
* 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]>
@markoburcul
Copy link
Contributor

The pipeline is successfully up and running. I've enabled status-web repo on ghcmgr and I did some modifications on jenkins-lib in order to make nix shell steps look cleaner.

@felicio
Copy link
Collaborator Author

felicio commented Oct 10, 2024

@markoburcul why didn't this #600 PR get built?

@felicio felicio reopened this Oct 10, 2024
@markoburcul
Copy link
Contributor

@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!

@felicio
Copy link
Collaborator Author

felicio commented Oct 10, 2024

@markoburcul thank you very much. One more thing, those builds should really just be for that directory and not for #602 (comment), for example.

@markoburcul
Copy link
Contributor

@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 status-im-auto on the PR? Are you using it for Q/A? If not, there is also a check in the bottom of the PR which reports if build has succeeded or failed.

markoburcul added a commit that referenced this issue Oct 11, 2024
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]>
@felicio
Copy link
Collaborator Author

felicio commented Oct 11, 2024

@markoburcul was about to just reply before you commented and now deleted your message 🙂

I've tried on the stage level but it won't work first time the PR

Finishing early wouldn't work?

pipeline {
    agent any
    stages {
        stage('Check Changed Files') {
            steps {
                script {
                    // Get the list of changed files
                    def changedFiles = sh(
                        script: "git diff --name-only origin/main...HEAD",
                        returnStdout: true
                    ).trim()

                    // Define paths or files that should trigger the build
                    def relevantPaths = ['src/', 'config/']

                    // Check if the changes are relevant
                    def isRelevant = changedFiles.split('\n').any { file ->
                        relevantPaths.any { path -> file.startsWith(path) }
                    }

                    // If no relevant changes, skip the build
                    if (!isRelevant) {
                        echo 'No relevant changes, skipping the job...'
                        currentBuild.result = 'SUCCESS'
                        return // Exit the pipeline early
                    } else {
                        echo 'Relevant changes detected, proceeding with the build...'
                    }
                }
            }
        }
...

– source, chatgpt

Why do you need comments from status-im-auto on the PR?

For consistency with other repos like desktop and mobile.

Are you using it for Q/A?

For example.

@markoburcul
Copy link
Contributor

@markoburcul was about to just reply before you commented and now deleted your message 🙂

Sorry for that, I realized my solution wasn't working as expected!

Finishing early wouldn't work?

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.

For consistency with other repos like desktop and mobile.

Sure.

@felicio
Copy link
Collaborator Author

felicio commented Oct 11, 2024

It would but for other people with opened PR's the check down below would be failed.

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

I'm affraid the only option is to skip stages after.

That's what I meant by finishing early and not executing reaming steps from the Jenkins pipeline (i.e. build, zip, comment).

@markoburcul
Copy link
Contributor

Here is the PR that does the trick, thanks for the suggestion!

@felicio
Copy link
Collaborator Author

felicio commented Oct 11, 2024

Image
#615

Thank you @markoburcul

@felicio felicio closed this as completed Oct 11, 2024
@felicio
Copy link
Collaborator Author

felicio commented Oct 17, 2024

Image

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

@markoburcul
Copy link
Contributor

Image

@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 apps/connector directory, which is the directory where we are checking if any change happened. Could it be that you rebased the branch or merged changes from main?

@felicio
Copy link
Collaborator Author

felicio commented Oct 17, 2024

@markoburcul

Could it be that you rebased the branch or merged changes from main?

Nope. GitHub would show. Something else gotta be triggering those kind of builds, which eventually produces those detached/not searchable commits

Merging remotes/origin/main commit 1b05730c3d282f20cab0a147bde0207a944fc29d into PR head commit 152e00381c99cdcea498d5edb5083fa34cd4d34e
Merge succeeded, producing 570a13c7173b24d00b827c3e49adcb26af282c5f

https://ci.status.im/job/status-web/job/prs/job/linux/job/PR-600/4/console

@markoburcul
Copy link
Contributor

Nope. GitHub would show. Something else gotta be triggering those kind of builds, which eventually produces those detached/not searchable commits

Merging remotes/origin/main commit 1b05730c3d282f20cab0a147bde0207a944fc29d into PR head commit 152e00381c99cdcea498d5edb5083fa34cd4d34e
Merge succeeded, producing 570a13c7173b24d00b827c3e49adcb26af282c5f

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?

@felicio
Copy link
Collaborator Author

felicio commented Oct 17, 2024

@markoburcul
Copy link
Contributor

PRs? There hasn't been any other for that directory open. Builds? All but the first one.

* https://ci.status.im/job/status-web/job/prs/job/linux/job/PR-600/2/

* https://ci.status.im/job/status-web/job/prs/job/linux/job/PR-600/3/

* https://ci.status.im/job/status-web/job/prs/job/linux/job/PR-600/4/

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 main branch and see if it would trigger a new build on the pr.

@felicio
Copy link
Collaborator Author

felicio commented Oct 17, 2024

You could test it by opening new pr with a change in apps/connector dir

Did you 😉?

@markoburcul
Copy link
Contributor

You could test it by opening new pr with a change in apps/connector dir

Did you 😉?

I've opened the pr and now I'll wait for any change in the main

@felicio
Copy link
Collaborator Author

felicio commented Oct 17, 2024

Merged a commit into main.

@felicio
Copy link
Collaborator Author

felicio commented Oct 17, 2024

But I see your PR wasn't even built #620.

@felicio felicio reopened this Oct 17, 2024
@markoburcul
Copy link
Contributor

But I see your PR wasn't even built #620.

In my PR I explained the behaviour #622

@markoburcul
Copy link
Contributor

@felicio could you take a look at my PR so we can merge it? Since the changing of the strategy in Jenkins settings will result in all other PR's that don't have this Jenkinsfile change to fail as here

@markoburcul
Copy link
Contributor

@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.
So we managed to get desired behaviour:

  • the PR gets comment from autobot only when the change happens in the apps/connector directory
  • builds are issued only when change occurs on the PR

@felicio
Copy link
Collaborator Author

felicio commented Oct 17, 2024

Created #626 to see if it will build only once and remain so overtime as per the original issue.

@markoburcul
Copy link
Contributor

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.

@markoburcul
Copy link
Contributor

Created #626 to see if it will build only once and remain so overtime as per the original issue.

Could you check my last comment? I believe that we achieved the desired behavior.

@felicio
Copy link
Collaborator Author

felicio commented Oct 18, 2024

No new unrelated builds in #600 (comment) and #626 (comment).

Closing. Thanks again @markoburcul .

@felicio felicio closed this as completed Oct 18, 2024
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

No branches or pull requests

3 participants