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

Check Echidna publishing results for failure or success #1821

Merged
merged 20 commits into from
Oct 4, 2024

Conversation

dschuff
Copy link
Member

@dschuff dschuff commented Oct 3, 2024

The API call does not return its results synchronously, so the script polls for the result and
exits with an error if it fails.
This PR also introduces the ability to run the publishing step as a dry run, so it can be
used with pull requests.

@dschuff
Copy link
Member Author

dschuff commented Oct 3, 2024

Hm, I forgot that secrets aren't passed to PRs that come from forks. It's a bummer that Echidna requires a token even for dry runs.

@dschuff
Copy link
Member Author

dschuff commented Oct 3, 2024

The current version of the workflow can use either the Echidna tokens (stored as a secret on WebAssembly/spec) or personal W3C username/password (which are handy for testing in forks, since anyone can put their own credentials as a secret in their own fork). Unfortunately even if a user has their W3C account info in their fork, it still doesn't get passed to workflows in PRs against upstream. I haven't yet figured out a way to get user credential information into PRs against upstream (we can't get and don't want our echidna token exposed to PRs from random forks).
Still, I think this PR is an improvement over the status quo. It does actually expose when our specs fail to validate and publish.

@dschuff dschuff marked this pull request as ready for review October 3, 2024 22:37
@dschuff dschuff requested review from rossberg, sideshowbarker and Ms2ger and removed request for rossberg and sideshowbarker October 3, 2024 22:37
W3C_ECHIDNA_TOKEN_CORE: ${{ secrets.W3C_ECHIDNA_TOKEN_CORE }}
W3C_ECHIDNA_TOKEN_JSAPI: ${{ secrets.W3C_ECHIDNA_TOKEN_JSAPI }}
W3C_ECHIDNA_TOKEN_WEBAPI: ${{ secrets.W3C_ECHIDNA_TOKEN_WEBAPI }}
YARN_ENABLE_IMMUTABLE_INSTALLS: false
ECHIDNA_DRYRUN: ${{ github.event_name == 'pull_request' && github.repository == 'WebAssembly/spec'}}
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused, why do a dry run when it is a PR against the main repo? Shouldn't it be the other way round? In fact, shouldn't it be a dry run unless it is an actual push against the main repo?

Also, this flag is set for the WD-echidna-CI target, but in two of the makefiles it's only used by the WD-echidna target.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah oops, I made it supported in more places, changed the default in the Makefile to true, and inverted most of the logic after it was originally written, but missed this.

Yes, it should be a dryrun for every case other than a push in the WebAssembly repo. Maybe I'll also add a condition on the branch name, just in case we find it useful for testing other branches in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note though that as I mentioned yesterday, this doesn't actually work at all yet for PRs against upstream that originate from a fork, because those don't get repository secrets from either the source or the destination repo.

document/js-api/Makefile Show resolved Hide resolved
document/web-api/Makefile Show resolved Hide resolved
@dschuff dschuff merged commit 2d480da into WebAssembly:main Oct 4, 2024
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