Repository navigation
[No QA] Trigger artifacts workflow when artifacts workflow files change - #103149
roryabraham merged 2 commits into
Conversation
mateuuszzzzz
changed the title
mikolajpochec
left a comment
mikolajpochec
left a comment
There was a problem hiding this comment.
Overall LGTM, but I posted two discussion points.
| id: didWorkflowsChange | ||
| run: | | ||
| # Workflow edits (e.g. the Hermes version resolution) change how artifacts are built without touching patches or the RN version. | ||
| if git diff --quiet '${{ github.event.before }}' '${{ github.event.after }}' -- .github/workflows/publishReactNativeArtifacts.yml .github/workflows/publishReactNativeiOSArtifacts.yml .github/workflows/publishReactNativeAndroidArtifacts.yml; then |
There was a problem hiding this comment.
Maybe it's out of scope of this PR, but what do you think about triggering this on diff of these files:
scripts/artifacts-utils/ios/publish/**,.github/actions/composite/fetchReactNativePrebuild/action.yml,.github/actions/composite/getXcodeVersion/action.yml?
They affect the build too, but don't trigger rebuilding of artifacts.
There was a problem hiding this comment.
This is a very good point! I'll verify those suggestions on Monday. Possibly we don't need to rely on .github/actions/composite/getXcodeVersion/action.yml, but I'm not sure about other paths you mentioned
There was a problem hiding this comment.
@mikolajpochec so I tried to walk this path, but it turned out to be really complicated. I extended my thinking about this feature a bit as well so I will share all my reasoning for clarification:
-
If we trigger builds based on changes in workflow files, should we include them in patches hash as well?
- In my opinion, no, because those changes in 99% do not break build and what's more still we look for the matching build by traversing published packages from newest to oldest so newest matching will be picked. Also, I think we even don't have a trivial way to match that information on client side(build that consumes artifacts) reliably.
-
I decided not to rely on toolchain versions (e.g.
.github/actions/composite/getXcodeVersion/action.yml) as a trigger condition, because if the build succeeds for a given target architecture, the build system should have no issues linking those artifacts correctly into the app no matter which toolchain version it uses locally. -
Historically issue with Hermes ABI mismatch was the first one that caused silent problems in builds using artifacts. This is project dependency so I decided to implement extra check here: [No QA] Implement Hermes version check for our custom artifacts #103317. This reasoning does not apply to CI workflows or utility scripts as they're not dependencies in terms of Gradle and Xcode builds
-
Nevertheless, if issues like the Hermes ABI mismatch arise and we need to adjust our CI artifacts pipeline in order to fix it, we would like to trigger new build automatically for such changes. This should be covered by additional trigger paths that include the files the workflows depend on. However, for the reasons mentioned above, I believe this should be handled only in the entry workflow file and nowhere else.
Obviously, these safeguards might not be sufficient in the future if subtle upstream changes affect how artifacts are built, but this is something we cannot entirely avoid in our custom pipeline. The iOS side of things is still very immature, and upstream is expected to drop CocoaPods in the near future. On the other hand, the Android side is very stable, and we’ve never had any issues with it.
|
@aimane-chnaif Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.mp4iOS: mWeb SafariMacOS: Chrome / Safari |
aimane-chnaif
left a comment
aimane-chnaif
left a comment
There was a problem hiding this comment.
PR description is out of date
The Explanation of Change still mentions only "the three existing artifact-related workflows". Since e27f58d it also covers fetchReactNativePrebuild/action.yml and scripts/artifacts-utils/ios/publish/**.
| - name: Check if artifact publishing workflows changed | ||
| if: ${{ github.event.after != '' }} | ||
| id: didWorkflowsChange | ||
| run: | | ||
| # Edits to the publishing workflows, the prebuild fetch action, or the iOS publish Gradle project | ||
| # change how artifacts are built without touching patches or the RN version. Keep in sync with `on.push.paths`. | ||
| if git diff --quiet '${{ github.event.before }}' '${{ github.event.after }}' -- \ | ||
| .github/workflows/publishReactNativeArtifacts.yml \ | ||
| .github/workflows/publishReactNativeiOSArtifacts.yml \ | ||
| .github/workflows/publishReactNativeAndroidArtifacts.yml \ | ||
| .github/actions/composite/fetchReactNativePrebuild/action.yml \ | ||
| scripts/artifacts-utils/ios/publish; then | ||
| echo "DID_WORKFLOWS_CHANGE=false" >> "$GITHUB_OUTPUT" | ||
| else | ||
| echo "DID_WORKFLOWS_CHANGE=true" >> "$GITHUB_OUTPUT" | ||
| echo "::notice::Detected changes in the artifact publishing workflows" |
There was a problem hiding this comment.
didWorkflowsChange no longer matches its scope
The step now also checks fetchReactNativePrebuild/action.yml and the iOS publish Gradle project, but its name, id and output still say "workflows". Line 167 then logs "a change to the publishing workflows" when the trigger was the Gradle project.
Rename to something like didPublishPipelineChange / DID_PUBLISH_PIPELINE_CHANGE, and update the reference in the comment at line 44.
| if git diff --quiet '${{ github.event.before }}' '${{ github.event.after }}' -- \ | ||
| .github/workflows/publishReactNativeArtifacts.yml \ | ||
| .github/workflows/publishReactNativeiOSArtifacts.yml \ | ||
| .github/workflows/publishReactNativeAndroidArtifacts.yml \ | ||
| .github/actions/composite/fetchReactNativePrebuild/action.yml \ | ||
| scripts/artifacts-utils/ios/publish; then | ||
| echo "DID_WORKFLOWS_CHANGE=false" >> "$GITHUB_OUTPUT" | ||
| else | ||
| echo "DID_WORKFLOWS_CHANGE=true" >> "$GITHUB_OUTPUT" | ||
| echo "::notice::Detected changes in the artifact publishing workflows" |
There was a problem hiding this comment.
A git diff error is treated as a change
git diff --quiet exits 128 on failure, e.g. a missing commit. Inside an if, bash -e doesn't apply, so the error falls into the else branch.
Expected: the step fails with the git error.
Actual: it logs "Detected changes in the artifact publishing workflows" and starts a full rebuild of all four jobs.
Suggested Fix: capture the exit code and fail when it's above 1.
aimane-chnaif
left a comment
aimane-chnaif
left a comment
There was a problem hiding this comment.
Left nab comments.
LGTM
ReviewI found no blocking issues. The new paths trigger the workflow, and a change in any of them forces a full rebuild of both targets. Republishing is safe because Notes
|
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.5.15-1 🚀
|
|
🚀 Deployed to production by https://github.com/grgia in version: 9.5.15-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
This is a follow-up PR that addresses a gap in our workflow trigger paths. Previously, changes to the artifact workflows would not trigger a new artifact build.
After this change is merged, we will regenerate the Standalone and HybridApp artifacts every time someone edits one of the three existing artifact-related workflows.
Fixed Issues
$ #100503
PROPOSAL:
Tests
This change cannot be tested easily, but it is possible. To verify it, one must copy this workflow to their fork and confirm that it is triggered by a push to main after merging a PR that modifies one of the three artifact workflow files.
An internal engineer can also test it in this repository by creating two branches and triggering the workflow by merging a PR into a new base branch. The second branch must modify one of the artifact workflow files.
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari