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

Enable chromatic turbosnap #2198

Merged
merged 2 commits into from
May 20, 2024

Conversation

yangwooseong
Copy link
Collaborator

@yangwooseong yangwooseong commented May 7, 2024

Self Checklist

  • I wrote a PR title in English and added an appropriate label to the PR.
  • I wrote the commit message in English and to follow the Conventional Commits specification.
  • I added the changeset about the changes that needed to be released. (or didn't have to)
  • I wrote or updated documentation related to the changes. (or didn't have to)
  • I wrote or updated tests related to the changes. (or didn't have to)
  • I tested the changes in various browsers. (or didn't have to)
    • Windows: Chrome, Edge, (Optional) Firefox
    • macOS: Chrome, Edge, Safari, (Optional) Firefox

Related Issue

Summary

  • Chromatic의 turbosnap기능을 활성화하고, trigger되는 조건을 push, pull_request에서 push로 변경합니다.

Details

  • turbosnap은 webpack의 디펜던시 그래프를 이용해서 변화가 있는 스토리만 스냅샷을 찍는 기능입니다. 베지어 스토리 갯수가 100개 정도 되서 사용량 제한 5000개를 초과하지 않기 위해 필요한 기능입니다. 자세한 설명은 이슈를 첨고해주세요.
  • Help컴포넌트를 바꾸는 경우: 디펜던시 그래프에서 Help와 연결된 부분 (Help, FormLabel, FormControl, Modal, Button 등)이 변화가 있다고 판단하고 관련된 스토리 22개만 스냅샷으로 찍습니다(#). Modal이나 Button이 연결되어 있다고 판단하는 것은 스토리에서 FormLabel, FormControl을 사용하고 있어서 그렇습니다.
  • AlphaButton 컴포넌트를 바꾸는 경우: AlphaButton에 의존하는 컴포넌트가 없기 때문에 스냅샷 하나만 찍게 됩니다 (#).
  • turbosnap문서에서 pull_request가 아닌 push를 이벤트 트리거로 사용하라고 권장하고 있어서 pull_request를 제거하고 브랜치 필터를 제거했습니다.

Breaking change? (Yes/No)

  • No

References

@yangwooseong yangwooseong added the enhancement Issues or PR related to making existing features better label May 7, 2024
@yangwooseong yangwooseong self-assigned this May 7, 2024
@yangwooseong yangwooseong requested a review from sungik-choi as a code owner May 7, 2024 00:53
Copy link

changeset-bot bot commented May 7, 2024

⚠️ No Changeset found

Latest commit: 5d0909b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

codecov bot commented May 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.10%. Comparing base (0e084eb) to head (5d0909b).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2198   +/-   ##
=======================================
  Coverage   85.10%   85.10%           
=======================================
  Files         133      133           
  Lines        2860     2860           
  Branches      864      864           
=======================================
  Hits         2434     2434           
  Misses        397      397           
  Partials       29       29           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@channel-io channel-io deleted a comment from channeltalk bot May 7, 2024
Comment on lines 5 to 8
branches:
- main
- alpha
pull_request:
Copy link
Contributor

Choose a reason for hiding this comment

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

이렇게 되면 모든 포크된 레포지토리의 push에 트리거될텐데 괜찮을까요?
기존 트리거 로직에도 github.repository == 'channel-io/bezier-react' 조건이 없었어서 포크된 레포지토리에서도 PR 생성/업데이트 시 트리거 됐을 거 같긴하지만요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

말씀하신대로 기존 트리거 로직도 포크된 repo의 pr에서도 크로마틱이 돌고 있고, push 이후 보통 pr을 만드니 push 이벤트가 기존의 트리거 로직과 크게 차이가 없을 것이라고 생각했습니다.

Copy link
Contributor

@sungik-choi sungik-choi May 7, 2024

Choose a reason for hiding this comment

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

PR에 포함되지 않은 포크된 repo의 커밋(푸시)에 대해서도 워크플로가 계속 트리거되어야하나 싶었어요. 크로마틱 토큰이 공개되어 있어서 의도만 있다면 악용될 여지는 항상 있겠지만, 모든 커밋에 대해서 트리거된다면 의도하지 않은 경우에도 로컬 테스트 등으로 스토리 스냅샷 양이 불필요하게 늘어나거나 할 수 있을 거 같아서요.

turbosnap문서에서 pull_request가 아닌 push를 이벤트 트리거로 사용하라고 권장하고 있어서

이 부분의 이유가 궁금했습니다. pull_request로 하면 어떤 문제가 발생하는지?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

공식 문서에서 pull_request 를 사용하면 diff 를 구하기 위해 조상을 찾지 못할 수 있다고 합니다.

pr 에 포함되지 않은 푸쉬에 대해서 크로마틱이 안돌게 하는 방법을 찾아볼게요.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

push로 하면 마지막 커밋이 packages/bezier-react/ 밖에서 발생하면 크로마틱 빌드가 트리거되지 않는 문제가 있긴 하네요(이슈). 한편으로는 https://github.com/guardian/dotcom-rendering 여기서 turbosnap 을 pull_request 이벤트와 함께 사용하는 히스토리가 있어서 이걸 이해하고 도입해보는 방향을 고려중입니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

GA 어렵네요...ㅎㅎ 저도 링크 확인해봤는데 좋은 거 같습니다! 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

pull_request 로 테스트해봤는데 큰 문제가 없는 것 같아서 괜찮을 것 같네요. (build 2894, 2895)

pull_request를 권장하지 않는 이유는, merge commit이 깃 히스토리에 없어서 특정 상황에 baseline을 잃어버릴 수 있어서 turbosnap 유무와 상관없이 크로마틱을 push이벤트를 사용할 것을 권장하고 있는데(#), 지금까지 pull_request로 �잘 사용하고 있었기에 괜찮을 것 같아요.

Copy link

channeltalk bot commented May 7, 2024

@yangwooseong yangwooseong force-pushed the WEB-767/enable-turbo-snap branch from 4afea60 to 5d0909b Compare May 20, 2024 04:32
@yangwooseong yangwooseong requested a review from sungik-choi May 20, 2024 05:07
@yangwooseong yangwooseong merged commit 8f32f50 into channel-io:main May 20, 2024
5 checks passed
yangwooseong added a commit that referenced this pull request May 31, 2024
<!--
  How to write a good PR title:
- Follow [the Conventional Commits
specification](https://www.conventionalcommits.org/en/v1.0.0/).
  - Give as much context as necessary and as little as possible
  - Prefix it with [WIP] while it’s a work in progress
-->

## Self Checklist

- [x] I wrote a PR title in **English** and added an appropriate
**label** to the PR.
- [x] I wrote the commit message in **English** and to follow [**the
Conventional Commits
specification**](https://www.conventionalcommits.org/en/v1.0.0/).
- [x] I [added the
**changeset**](https://github.com/changesets/changesets/blob/main/docs/adding-a-changeset.md)
about the changes that needed to be released. (or didn't have to)
- [x] I wrote or updated **documentation** related to the changes. (or
didn't have to)
- [x] I wrote or updated **tests** related to the changes. (or didn't
have to)
- [x] I tested the changes in various browsers. (or didn't have to)
  - Windows: Chrome, Edge, (Optional) Firefox
  - macOS: Chrome, Edge, Safari, (Optional) Firefox

## Related Issue

<!-- Please link to issue if one exists -->

<!-- Fixes #0000 -->

- #2246 와 관련있어보입니다. 

## Summary

<!-- Please brief explanation of the changes made -->

- main branch 푸쉬할 떄 크로마틱 빌드가 되지 않고 있습니다.
#2198 에서 잘못 삭제된 push 트리거를
되살립니다.

## Details

<!-- Please elaborate description of the changes -->

- None

### Breaking change? (Yes/No)

<!-- If Yes, please describe the impact and migration path for users -->

- No

## References

<!-- Please list any other resources or points the reviewer should be
aware of -->

- None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues or PR related to making existing features better
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Enable turbosnap when chromatic builds storybook
2 participants