Skip to content

Docs: fix continuous integration example#24054

Merged
jonniebigodes merged 8 commits into
storybookjs:nextfrom
natehouk:next
Oct 11, 2023
Merged

Docs: fix continuous integration example#24054
jonniebigodes merged 8 commits into
storybookjs:nextfrom
natehouk:next

Conversation

@natehouk
Copy link
Copy Markdown
Contributor

@natehouk natehouk commented Sep 2, 2023

What I did

Update to fix an error due to fetch-depth not being set to 0. With the current example, the following error occurs:

Run chromaui/action@v1

Chromatic CLI v6.24.1
https://www.chromatic.com/docs/cli

Authenticating with Chromatic
    → Connecting to https://index.chromatic.com/
Authenticated with Chromatic
    → Using project token '****************b2a6'
Retrieving git information
    → ✖ Found only one commit
This typically means you have ran into one of the following scenarios:
- You've checked out a shallow copy of the Git repository, which actions/checkout@v2 does by default.
  In order for Chromatic to correctly determine baseline commits, we need access to the full Git history graph.
  With actions/checkout@v2, you can enable this by setting 'fetch-depth: 0'.
  ℹ Read more at https://www.chromatic.com/docs/github-actions

This PR also switches the example from yarn to npm, but that can be reverted if necessary. However it is likely that most beginners following the tutorial are using npm and not yarn.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

Trigger example GitHub action .github/workflows/chromatic.yml as exemplified in documentation and ensure it runs without error.

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

Update to fix errors with fetch-depth
@natehouk natehouk changed the title Update chromatic-github-action.js.mdx Docs: Fix continuous integration example Sep 2, 2023
@natehouk natehouk changed the title Docs: Fix continuous integration example Docs: fix continuous integration example Sep 2, 2023
@jonniebigodes jonniebigodes self-assigned this Sep 2, 2023
@jonniebigodes jonniebigodes added documentation ci:docs Run the CI jobs for documentation checks only. labels Sep 2, 2023
Copy link
Copy Markdown
Contributor

@jonniebigodes jonniebigodes left a comment

Choose a reason for hiding this comment

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

@natehouk appreciate you taking the time to put together this pull request and catching this small but rather important typo in the documentation 🙏 ! Left one small item for you to look at when you have a moment.

Let me know, and we'll take it from there.

Hope you have a fantastic week.

Stay safe

Comment on lines +20 to +23
- uses: actions/setup-node@v3
with:
node-version: 20
cache: 'npm'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@natehouk when you're able, can you revert this to include yarn? Also, if you're going to reference a node version, could you instead point it to the stable release, as not all users are already on version 20, which could introduce some unwanted issues?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jonniebigodes I don't think I was able to get it to work with yarn, I think that is why I switched it back to npm.

Regardless, I made the two changes you requested:

  • Pinned to node version 18
  • Reverted to use yarn

Please check and merge.

Copy link
Copy Markdown
Contributor

@jonniebigodes jonniebigodes left a comment

Choose a reason for hiding this comment

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

@natehouk one small item and this should be good to go. Let me know once you've addressed it and I'll gladly merge it.

Have a fantastic week.

Stay safe

with:
node-version: 18
cache: 'yarn'
- run: yarn ci
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@natehouk can you remove the ci option from the yarn command, as there's no option for it, and as this is a CI workflow, it will automatically set up the immutable that operates similarly to the npm counterpart.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@natehouk Sorry for not being explicit. What I meant to say was to remove only the ci option, as you'll need to run the yarn command to install the dependencies.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jonniebigodes Fixed.

I added back this line:

- run: yarn

Let me know if this is good now. Thanks.

@natehouk
Copy link
Copy Markdown
Contributor Author

@natehouk one small item and this should be good to go. Let me know once you've addressed it and I'll gladly merge it.

Have a fantastic week.

Stay safe

@jonniebigodes I have removed the line as requested.

Copy link
Copy Markdown
Contributor

@jonniebigodes jonniebigodes left a comment

Choose a reason for hiding this comment

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

@natehouk thanks for addressing the feedback so promptly. Appreciate it 🙏 ! We're in the clear. I'll get it merged once the checklist goes through.

Have a fantastic day.

Stay safe

@jonniebigodes jonniebigodes merged commit 0b5697f into storybookjs:next Oct 11, 2023
@github-actions github-actions Bot mentioned this pull request Oct 11, 2023
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:docs Run the CI jobs for documentation checks only. documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants