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

storybook: add build job #6908

Merged
merged 6 commits into from
Jul 24, 2024
Merged

storybook: add build job #6908

merged 6 commits into from
Jul 24, 2024

Conversation

VIKTORVAV99
Copy link
Member

@VIKTORVAV99 VIKTORVAV99 commented Jun 27, 2024

Issue

If we want to use Storybook effectively we need it in the CI/CD pipeline.

Closes: AVO-349

Description

This adds a build job for storybook which upload a build artefact. I will create a separate CI job that uploads this artefact to Cloudflare.

It also creates a new vite config for storybook so we don't build it with sentry and PWA stuff it was not designed to handle.

Double check

  • I have run pnpx prettier@2 --write . and poetry run format in the top level directory to format my changes.

@madsnedergaard
Copy link
Member

I like the idea, but wouldn't it be easier to do it with Earthly similar to how we deploy the app?

@VIKTORVAV99
Copy link
Member Author

I like the idea, but wouldn't it be easier to do it with Earthly similar to how we deploy the app?

I wanted to keep as much of the "contrib" stuff in GitHub as possible if that makes sense. And we are already deploying the staging and previews like this. The only difference is that this adds a intermediate build job so we can check that the build is actually passing (we have cypress for that on the other web stuff) and then reusing the same artefacts for deployment (next steps) or you can download them and open them in vite preview for example.

@madsnedergaard
Copy link
Member

I like the idea, but wouldn't it be easier to do it with Earthly similar to how we deploy the app?

I wanted to keep as much of the "contrib" stuff in GitHub as possible if that makes sense. And we are already deploying the staging and previews like this. The only difference is that this adds a intermediate build job so we can check that the build is actually passing (we have cypress for that on the other web stuff) and then reusing the same artefacts for deployment (next steps) or you can download them and open them in vite preview for example.

Fair, that makes sense (keeping in in GH Actions).

Is it really worth running the job on every single PR instead of just on merges to master, where we would deploy it on a real URL somewhere (like ui.electricitymaps.com?)?
I struggle seeing the usability of having a built version available on every single PR, and it adds up on CI time and build minutes - but happy to hear your thoughts on this :)

@VIKTORVAV99
Copy link
Member Author

VIKTORVAV99 commented Jul 3, 2024

I like the idea, but wouldn't it be easier to do it with Earthly similar to how we deploy the app?

I wanted to keep as much of the "contrib" stuff in GitHub as possible if that makes sense. And we are already deploying the staging and previews like this. The only difference is that this adds a intermediate build job so we can check that the build is actually passing (we have cypress for that on the other web stuff) and then reusing the same artefacts for deployment (next steps) or you can download them and open them in vite preview for example.

Fair, that makes sense (keeping in in GH Actions).

Is it really worth running the job on every single PR instead of just on merges to master, where we would deploy it on a real URL somewhere (like ui.electricitymaps.com?)? I struggle seeing the usability of having a built version available on every single PR, and it adds up on CI time and build minutes - but happy to hear your thoughts on this :)

Running it on each PR means we can get preview builds as well, at least when someone in the Emaps org opens a PR which could be nice when reviewing PRs and just checking general changes.

But if we just want to publish on master we can do that, I would however still want to build the project so we ensure we don't do breaking changes to the storybook config in some way.

As for the URL I was thinking either storybook.electricitymaps.com or design.electricitymaps.com but ui could work as well.

@madsnedergaard
Copy link
Member

I like the idea, but wouldn't it be easier to do it with Earthly similar to how we deploy the app?

I wanted to keep as much of the "contrib" stuff in GitHub as possible if that makes sense. And we are already deploying the staging and previews like this. The only difference is that this adds a intermediate build job so we can check that the build is actually passing (we have cypress for that on the other web stuff) and then reusing the same artefacts for deployment (next steps) or you can download them and open them in vite preview for example.

Fair, that makes sense (keeping in in GH Actions).
Is it really worth running the job on every single PR instead of just on merges to master, where we would deploy it on a real URL somewhere (like ui.electricitymaps.com?)? I struggle seeing the usability of having a built version available on every single PR, and it adds up on CI time and build minutes - but happy to hear your thoughts on this :)

Running it on each PR means we can get preview builds as well, at least when someone in the Emaps org opens a PR which could be nice when reviewing PRs and just checking general changes.

But if we just want to publish on master we can do that, I would however still want to build the project so we ensure we don't do breaking changes to the storybook config in some way.

Okay fair enough, we can always reconsider if this becomes a problem :)

As for the URL I was thinking either storybook.electricitymaps.com or design.electricitymaps.com but ui could work as well.

Could also work, I guess "design.electricitymaps.com" could work nicely for using it as a wider brand guidelines, where we also keep logos, colors, etc. clearly defined :)

Copy link
Member

@madsnedergaard madsnedergaard left a comment

Choose a reason for hiding this comment

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

LGTM! :)

@VIKTORVAV99 VIKTORVAV99 enabled auto-merge (squash) July 24, 2024 21:47
@VIKTORVAV99 VIKTORVAV99 merged commit 9964016 into master Jul 24, 2024
21 checks passed
@VIKTORVAV99 VIKTORVAV99 deleted the vik/build_storybook_in_ci branch July 24, 2024 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file frontend 🎨 infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants