Skip to content

Conversation

@eyelidlessness
Copy link
Contributor

@eyelidlessness eyelidlessness commented Apr 10, 2025

Fixes failure to produce a build artifact on merge to main. (Or at least, I think it fixes that!)

Closes # (do we have an issue for this?)

I have verified this PR works in these browsers (latest versions):

  • Chrome
  • Firefox
  • Safari (macOS)
  • Safari (iOS)
  • Not applicable
  • GitHub Actions

What else has been done to verify that this works as intended?

After the first failed attempt in #368, I did a bunch of trial and error in a private personal repo. Note: I haven't validated the fork-specific logic (because no fork of private personal repo)! But I have validated that jobs predicated on that condition run as expected (and don't run for a negative predicate result).

Why is this the best possible solution? Were any other approaches considered?

It probably isn't the best possible solution!

  • I briefly considered investigating other CI solutions but we don't want that.
  • We discussed making the build -> upload artifact job more explicit. I think we should do that but don't want to block a pragmatic fix over it.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

I could imagine a few hundred ways this breaks assumptions we have about CI. But the real risk is the ways I can't imagine. Such is the nature of GitHub Actions!

Do we need any specific form for testing your changes? If so, please attach one.

N/A

What's changed

CI will now be skipped if triggered by a pull_request event, except if the PR is from a fork... where "from a fork" is defined as repository_owner being some value other than 'getodk'.

@changeset-bot
Copy link

changeset-bot bot commented Apr 10, 2025

⚠️ No Changeset found

Latest commit: 8bca260

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

@eyelidlessness eyelidlessness marked this pull request as ready for review April 10, 2025 19:09
@eyelidlessness eyelidlessness force-pushed the fix/ci-upload-dist-demo-artifact branch from 256e84e to 8bca260 Compare April 10, 2025 19:12
Copy link
Collaborator

@latin-panda latin-panda left a comment

Choose a reason for hiding this comment

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

It ends up like:
Non-forked runs push
Forked runs pull_request and push

LGTM! Ready to merge

@eyelidlessness eyelidlessness merged commit 23d120d into main Apr 10, 2025
47 checks passed
@eyelidlessness eyelidlessness deleted the fix/ci-upload-dist-demo-artifact branch April 10, 2025 20:22
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