-
Notifications
You must be signed in to change notification settings - Fork 16
[FF-50] Picasso Storybook docs deployment #4814
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
base: master
Are you sure you want to change the base?
Conversation
|
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
.github/workflows/ci.yaml
Outdated
| deploy-storybook-preview: | ||
| if: ${{ github.event.pull_request.head.ref != 'changeset-release/master' }} | ||
| name: Deploy Picasso docs | ||
| name: Deploy Storybook Preview |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest not to change them, as they can be mentioned in some Grafana dashboards or some reports
.github/workflows/ci.yaml
Outdated
| body: '📖 **Storybook Preview**\n\n🚀 Your Storybook preview is ready: **[View Storybook](' + deployUrl + ')**\n\n📍 Preview URL: `' + deployUrl + '`\n\nThis preview is updated automatically when you push changes to this PR.' | ||
| }); | ||
| cleanup-storybook-preview: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why we would even bother removing it if we use gh pages? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we need to update whole directory each time, if we will not clean it checkout and update time will grow. One thing I need to check about concurrency between parallel PRs and master when playing with gh-pages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it 🤔 yeah, but this doesn't look reliable. Maybe we can have a separate workflow, that will be triggered on CRON? because this one can fail or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, need to test.
Regarding concurrency I'm will add concurrent group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, I mean, imagine you close the PR and this workflow failed by some reason, timeout, ex. So PR temploy will be forever in the repo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could happen indeed, but let's see maybe it something we can fix down the road. For me right now what is important is multiple instances of docs (release, temploy) and concurrency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be solved by separate workflow run on cron schedule ☝️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, separate workflow that reacts on: pr closed, cron job and can also be manually triggered.
.github/workflows/release.yml
Outdated
| # Backup PR preview directories | ||
| mkdir -p pr-backups | ||
| find gh-pages-content -maxdepth 1 -name "pr-*" -type d -exec cp -r {} pr-backups/ \; 2>/dev/null || true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we have 2 separate directories: for PRs and for master? or we have to use only one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems we can have only one but I can check would it simplify to have one more level with 2 directories
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified by introducing /prs directory so preview url will be prs/123
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
denieler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good 👍
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
FF-50
Description
Deploy Storybook docs to GH pages:
How to test
Screenshots
Development checks
picasso-tailwind-mergerequires major update (check itsREADME.md)propsin component with documentationexamplesfor componentBreaking change
PR commands
List of available commands:
@toptal-bot run package:alpha-release- Release alpha version@toptal-anvil ping reviewers- Ping for reviewsPR Review Guidelines
When to approve? ✅
You are OK with merging this PR and
nit:to your comment. (ex.nit: I'd rename this variable from makeCircle to getCircle)When to request changes? ❌
You are not OK with merging this PR because
When to comment (neither ✅ nor ❌)
You want your comments to be addressed before merging this PR in cases like:
How to handle the comments?