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

jenkinsfile: revert check for changed files #622

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

markoburcul
Copy link
Contributor

Reverted the check for changes in the apps/connector directory.

What we were missing before with this kind of check is this setting in the Jenkins pipeline:
Screenshot from 2024-10-17 11 00 14
and as explained here:
Screenshot from 2024-10-17 11 00 25

This allows, when PR is opened, that changelog exists and we don't need to execute git commands manually within jenkinsfile which is prone to errors.

The motivation for this was that previously the Strategy for Discover pull requests from origin was Merging the pull request with the current target branch revision and this caused the PR builds to happen when there were changes in the main:

Merging the pull request with the current target branch revision
    Discover each pull request once with the discovered revision corresponding to the result of merging with the current revision of the target branch. Note that pushes to the target branch will result in new pull request builds. 

I've changed this to:

The current pull request revision
    Discover each pull request once with the discovered revision corresponding to the pull request head revision without merging. 

But with this change a normal PR build would fail on our Check Changed Files stage with:

10:49:49  + git diff --name-only origin/main...HEAD
10:49:49  fatal: ambiguous argument 'origin/main...HEAD': unknown revision or path not in the working tree.
10:49:49  Use '--' to separate paths from revisions, like this:
10:49:49  'git <command> [<revision>...] -- [<file>...]'

because there is an uncomplete fetch of changes.

Signed-off-by: markoburcul <marko@status.im>
Copy link

changeset-bot bot commented Oct 17, 2024

⚠️ No Changeset found

Latest commit: 6404ea7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Oct 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
status-components ⬜️ Ignored (Inspect) Oct 17, 2024 9:18am

@status-im-auto
Copy link
Member

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 6404ea7 #1 2024-10-17 09:19:16 ~31 sec chrome 📦zip

@felicio
Copy link
Collaborator

felicio commented Oct 17, 2024

I don't know enough about this at the moment. Seems like a good find at first glance, but also don't know if it's not going start building unrelated PRs again. Maybe keeping both the setting and check could do, but approving so you can continue testing.

@markoburcul markoburcul merged commit 51d6189 into main Oct 17, 2024
6 checks passed
@felicio
Copy link
Collaborator

felicio commented Oct 17, 2024

I don't know enough about this at the moment. Seems like a good find at first glance, but also don't know if it's not going start building unrelated PRs again. Maybe keeping both the setting and check could do, but approving so you can continue testing.

@markoburcul please, see #591 (comment) where unrelated PR was built.

@markoburcul
Copy link
Contributor Author

I don't know enough about this at the moment. Seems like a good find at first glance, but also don't know if it's not going start building unrelated PRs again. Maybe keeping both the setting and check could do, but approving so you can continue testing.

@markoburcul please, see #591 (comment) where unrelated PR was built.

I added also this in the Jenkins settings for this pipeline:
Screenshot from 2024-10-17 13 54 07

Since I saw this pr wasn't even visible in the list of PRs of Jenkins UI. Do you want to enable the builds from forks that are submitted as PRs or no?

@felicio
Copy link
Collaborator

felicio commented Oct 17, 2024

@markoburcul for trusted ones yes. But how is it related to the original issue? That PR has no relevant changes. Maybe you shouldn't have removed the changes as well, as I raised in #622 (comment).

@markoburcul
Copy link
Contributor Author

@markoburcul for trusted ones yes. But how is it related to the original issue? That PR has no relevant changes. Maybe you shouldn't have removed the changes as well, as I raised in #622 (comment).

Because I scanned the repository and Jenkins automatically fetched this PR. It contains the old Jenkinsfile and thus no filtering was applied to stages. It should be updated with latest changes in main to skip stages with building and commenting on pr.

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.

3 participants