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

[ci] Fix files_changed merge main issues and files_changed and check_bazel script's base_sha to point to HEAD~1 #32

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

pablo-guardiola
Copy link
Contributor

Follow up from #24 and #28

This PR fixes:

Check for workflow file changes
Run ./ci/files_changed.sh .github/workflows/android.yaml
fatal: ambiguous argument 'origin/': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
No relevant changes found.
Check for relevant Gradle changes
Run ./ci/files_changed.sh "^platform/jvm/gradle-test-app/.*\.(gradle|kts|kt|xml)$"
fatal: ambiguous argument 'origin/': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
No relevant changes found.

This is solved by fixing files_changed to differentiate when you are in a PR or main, as well as ensuring git diff failures are caught and reported accordingly.

  • check_bazel and files_changed script's base_sha to point to HEAD~1 when running on a main merge. I noticed that no changes were detected even though they were present as part of the PR. Root cause was that we were diffing pointing to the same references and hence no changes. Pointing to the commit before the current one (HEAD~1) fixes this. I've thoroughly tested locally and now changes are detected too after merging a PR.

cc @murki

…as well as fix check_bazel and files_changed base_sha to point to HEAD~1 when running on a main merge
@murki
Copy link
Contributor

murki commented Sep 10, 2024

Thanks @pablo-guardiola , would you mind add some inconsequential code change somewhere that would be picked-up by files_changed.sh to verify it works? I can then approve and run the checks

@pablo-guardiola
Copy link
Contributor Author

@murki

would you mind add some inconsequential code change somewhere that would be picked-up by files_changed.sh to verify it works? I can then approve and run the checks

I already did that locally but I can push some "empty" changes for you to double check, not a problem.

It's important that the changes land on main to test if everything works after merging PRs and I didn't want to pollute the repo's commit history ¯\_(ツ)_/¯

But you can probably remove them quickly after confirming 👍

@@ -83,6 +83,8 @@ object Capture {
dateProvider: DateProvider? = null,
apiUrl: HttpUrl = defaultCaptureApiUrl,
) {
// TODO Adding a no-op for testing, please remove after confirming everything is good
val test = true
Copy link
Contributor

@murki murki Sep 10, 2024

Choose a reason for hiding this comment

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

this might break detekt, plus I agree with your statement about not polluting the repo with these changes and the TODOS since they will go into main. How about something inconsequential that can actually stay in teh codebase? Something like a variable rename for example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me. Let me quickly fix 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👉 ecd11ac

@murki murki enabled auto-merge (squash) September 10, 2024 17:37
@murki murki merged commit 467bbde into bitdriftlabs:main Sep 10, 2024
15 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 10, 2024
@pablo-guardiola pablo-guardiola deleted the pg-ci-fix-files-changed branch September 10, 2024 17:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants