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

Fix Github Actions issues. #3822

Merged
merged 2 commits into from
Jan 30, 2025

Conversation

brandonpage
Copy link
Contributor

  • Fix an issue where pull_request_target makes the default checkout ref the base branch instead of PR code.
  • Add workflow_dispatch trigger so nightly tests can be run from the browser.
  • Use Ruby gem cache.
  • Add a couple automated PR messages.

@brandonpage brandonpage requested review from Crebs and bbirman January 29, 2025 23:56
warn("Big PR, try to keep changes smaller if you can.", sticky: true) if git.lines_of_code > 1000

# Redirect contributors to PR to dev.
fail("Please re-submit this PR to the dev branch, we may have already fixed your issue.", sticky: true) if github.branch_for_base != "dev"
Copy link
Member

Choose a reason for hiding this comment

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

Does the whole check fail? Wondering about when we do PRs to master for cherry picks

Copy link
Contributor Author

@brandonpage brandonpage Jan 30, 2025

Choose a reason for hiding this comment

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

I believe it only fails the Danger status check on the PR. It should not stop the execution of tests or do much of anything else negative.

However, while writing this comment I realized it is actually completely pointless with the current configuration since CI is only triggered on dev. So I think we have two choices:

  1. Add master as a branch to trigger PRs. We probably want to keep the line then and ignore the fail. (Although I thought we typically just merge dev to master without a PR?).
  2. Remove the warning since CI doesn't run against master.

I think I like option # 1 more?

Copy link
Member

Choose a reason for hiding this comment

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

sounds good! it's been a while but this was the scenario I was thinking of #3686

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I double checked this on the Android testing setup. It just displays this error, which is fine in the rare event we are cherry picking to master.

Screenshot 2025-01-29 at 5 27 23 PM

@brandonpage brandonpage merged commit f74047f into forcedotcom:dev Jan 30, 2025
7 checks passed
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