Skip to content

Comments

Fix publish to upload wizened artifact#222

Merged
jeffcharles merged 1 commit intomainfrom
jc.fix-publish-wizened
Feb 1, 2023
Merged

Fix publish to upload wizened artifact#222
jeffcharles merged 1 commit intomainfrom
jc.fix-publish-wizened

Conversation

@jeffcharles
Copy link
Collaborator

I think the change in https://github.com/Shopify/javy/pull/205/files#diff-18497b72a2306fc2560475e2548cfe1b96b6206c395380437ba1fafefd66e126 introduced the problem. Currently testing in my personal fork to check that this fixes the issue before merging and cutting another release.

@jeffcharles
Copy link
Collaborator Author

My test publishing worked so this should fix the problem

Copy link
Member

@saulecabrera saulecabrera left a comment

Choose a reason for hiding this comment

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

As a follow up: I wonder if we should either set some env vars in this workflow that hold the values of all the different files that are used or at least document what goes where. There's a lot of cognitive overhead here and I'm worried that this will continue to happen.

@jeffcharles
Copy link
Collaborator Author

Agreed on there's excessively high cognitive overhead.

Could you elaborate on how using env vars would reduce that? Is the idea that we would use an optional env vars when invoking make core to set the names of the two final Wasm files and then we would reference those same env vars further down when uploading the Wasm files to the artifacts, and then again later when invoking cargo build?

My thinking is that it we may benefit from having a parallel workflow that's similar to the publish pipeline that runs during CI so if that breaks, the author and the reviewers realize that the actual publish workflow would need to be updated as well. We'd only need a job to compile the core Wasm modules on Linux with a similar artifact upload and then a job to download the two artifacts and ensure a cargo build (possibly could even get away with a faster cargo check) succeeds. I would argue that a failing PR check would signal the author of the PR to think about the potential impact of their change on the publish workflow.

@jeffcharles jeffcharles merged commit 6c79530 into main Feb 1, 2023
@jeffcharles jeffcharles deleted the jc.fix-publish-wizened branch February 1, 2023 14:41
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