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

Replace getUploadUrls with uploadBuild mutation #876

Conversation

ghengeveld
Copy link
Member

@ghengeveld ghengeveld commented Dec 13, 2023

Depends on https://github.com/chromaui/chromatic/pull/8049

This uses the new uploadBuild mutation which uses S3 presigned POST requests for uploading and enforces strict file size limitations.

The mutation no longer returns the storybook base domain, instead we query for storybookUrl on the build. All references to cachedUrl and isolatorUrl have been replaced with storybookUrl.

📦 Published PR as canary version: 10.1.1--canary.876.7220703629.0

✨ Test out this PR locally via:

npm install [email protected]
# or 
yarn add [email protected]

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

LGTM! I'm unsure exactly why we need two mutations however, why is the metadata separated from the build data (if it ends up in the same place)?

@ghengeveld
Copy link
Member Author

LGTM! I'm unsure exactly why we need two mutations however, why is the metadata separated from the build data (if it ends up in the same place)?

Because the metadata (CLI logs and such) is collected at the very end of the CLI process (when the build might've already finished). I suppose we could send part of the metadata along with the build upload itself (e.g. config files) but that would complicate things.

@ghengeveld ghengeveld added minor Auto: Increment the minor version when merged skip-release Auto: Preserve the current version when merged labels Dec 14, 2023
@andrewortwein
Copy link
Contributor

I am not able to run a build using the suggested command in the description. I have tried multiple projects, both existing and new, but the build always fails at the Storybook verifying step, just specifying that there was a failure to extract the Storybook. I also noticed that all of the smoke tests in this PR are failing, so I decided not to dig too far into the problems I'm seeing locally.

@ghengeveld ghengeveld added release Auto: Create a `latest` release when merged and removed skip-release Auto: Preserve the current version when merged labels Dec 15, 2023
@ghengeveld
Copy link
Member Author

@andrewortwein Sorry, I was using my local version rather than the canary, so I hadn't noticed the canary was very old and not updating anymore. That turned out to be due to the skip-release label on this PR. I changed it and reran the release action, and now we have an up-to-date canary that I confirmed works correctly.

@ghengeveld ghengeveld added skip-release Auto: Preserve the current version when merged and removed release Auto: Create a `latest` release when merged labels Dec 15, 2023
Copy link
Contributor

@andrewortwein andrewortwein left a comment

Choose a reason for hiding this comment

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

LGTM 🆕👍🏻

@ghengeveld ghengeveld added this pull request to the merge queue Dec 15, 2023
Merged via the queue into main with commit fa2d355 Dec 15, 2023
19 checks passed
@ghengeveld ghengeveld deleted the ghengeveld/ap-3909-cli-replace-getbuilduploadurls-with-uploadbuild-mutation branch December 15, 2023 19:10
@ghengeveld
Copy link
Member Author

🚀 PR was released in v10.2.0 🚀

@ghengeveld ghengeveld added the released Verdict: This issue/pull request has been released label Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Auto: Increment the minor version when merged released Verdict: This issue/pull request has been released skip-release Auto: Preserve the current version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants