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

[FEAT] CI / CD 환경 설정 #40

Merged
merged 11 commits into from
Jul 21, 2024
Merged

[FEAT] CI / CD 환경 설정 #40

merged 11 commits into from
Jul 21, 2024

Conversation

novice0840
Copy link
Contributor

Issue Number

#39

As-Is

Git을 통해 commit을 날리거나 push를 할 때 몇가지 조건 들이나 절차가 있는데 이것들을 일일히 수동으로 해줘야해서 불편했던 상황

To-Be

자동화 기능들이 몇 가지 추가 되었다.

  1. commit 시 메시지 맨 끝에 자동으로 현재 브랜치에 있는 이슈 번호가 부착된다.
  2. push시 pre-push라는 git hook을 이용해 develop 브랜치와의 rebase를 자동으로 해준다.
  3. PR 제출 시 소스 코드를 build, test 해서 통과해야 PR이 에러 없이 보내진다.

Check List

  • 테스트가 전부 통과되었나요?
  • 모든 commit이 push 되었나요?
  • merge할 branch를 확인했나요?
  • Assignee를 지정했나요?
  • Label을 지정했나요?

Test Screenshot

(Optional) Additional Description

@novice0840 novice0840 added ✅ test 테스트 관련 🗺 infra 인프라 관련 🫧 FE front end labels Jul 20, 2024
@novice0840 novice0840 added this to the FE Sprint2 milestone Jul 20, 2024
@novice0840 novice0840 linked an issue Jul 20, 2024 that may be closed by this pull request
3 tasks
Copy link
Contributor

@rbgksqkr rbgksqkr left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 포메 🐶🐶🐶
rebase, github actions 등 불편한 부분들을 자동화해주셔서 앞으로의 개발 생산성에 좋은 영향을 받을 것 같아요! 몇가지 코멘트 남겨봤으니 확인해주세용

- name: Use Node.js
uses: actions/setup-node@v3
with:
node-version: "20"
Copy link
Contributor

Choose a reason for hiding this comment

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

L4 - 변경 제안

노드 버전을 LTS 최신버전으로 선택하셨군요! 20으로 결정한 이유가 있을까요?? 릴리즈 노트를 보면 20은 아직 개발 단계로 보여지는데 18 버전을 사용하는 것은 어떻게 생각하시나요??

node 20을 지원하지 않는 os가 존재하는 것 같긴 한데 action 돌릴 노드 환경이니 상관없을 것 같긴 합니다. 그런데 제 로컬환경은 18.17.1 버전이라 저희 node 환경을 맞춰봐도 좋을 것 같네요!!!

포메의 생각이 궁금합니다!

https://nodejs.org/en/about/previous-releases
https://github.blog/changelog/2024-03-07-github-actions-all-actions-will-run-on-node20-instead-of-node16-by-default/

Copy link
Contributor

Choose a reason for hiding this comment

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

L4 - 변경 제안

추가적으로 github actions에 warning을 확인해보다가 action 레포를 들어가보니 아래와 같은 방식으로 의존성 경로를 캐싱할 수 있는 것 같더라구요. 참고하면 좋을 것 같습니다! 그리고 checkout과 setup-node 액션이 v4가 최신인 것 같아서 v4 를 사용하는 건 어떤가요??

steps:
- uses: actions/checkout@v4
- uses: actions/setup-node@v4
  with:
    node-version: 20
    cache: 'npm'
- run: npm ci
- run: npm test

https://github.com/actions/setup-node?tab=readme-ov-file#caching-global-packages-data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. LTS 버전인 20이 18보다 성능이 우수하기도 했고 베타버전이 아니므로 안정적이라 생각하여 사용하였습니다. 그리고 actions 내부에서만 확인하는 용도이므로 현재 actions의 환경인 우분투 최신버전에서는 정상 작동하므로 괜찮다고 생각합니다!

  2. 네 저도 최신 버전으로 하는 것이 좋을 것이라 생각해 최신 버전으로 변경하였습니다. 캐싱도 좋을 것 같아요!


- name: Test
run: npm test
working-directory: ./frontend
Copy link
Contributor

Choose a reason for hiding this comment

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

L5 - 참고 의견

나중에 파일 따로 분리해서 storybook 빌드랑 배포 관련한 스크립트도 만들면 좋을 것 같아요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

네, 추가로 넣으면 좋을 것 같아요! 아직 CD 부분은 구현이 안된 상태입니다!

@@ -1 +1,34 @@
cd frontend && npm run build-dev
# cd frontend && npm run build-dev
Copy link
Contributor

Choose a reason for hiding this comment

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

L5 - 참고 의견

npm run build-dev 작업을 github acions로 옮긴거죠?? 좋은 것 같아요! 아예 주석 부분 없애도 괜찮을 것 같습니당

Copy link
Contributor Author

Choose a reason for hiding this comment

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

네, 기존에 husky에서 확인하던 빌드 작업이 Github Actions로 옮겨갔습니다!

# 현재 브랜치 이름 가져오기
current_branch=$(git rev-parse --abbrev-ref HEAD)

# feat/ 로 시작하는 브랜치인지 확인
Copy link
Contributor

Choose a reason for hiding this comment

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

L4 - 변경 제안

오호 브랜치명도 제한을 두었군요! 저희 브랜치 컨벤션에 따라서 hotfix도 추가되면 좋을 것 같아요!

# 원래 브랜치로 돌아가기
git checkout $current_branch

# rebase 수행
Copy link
Contributor

Choose a reason for hiding this comment

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

L5 - 참고 의견

rebase 스크립트 만든 거 너무 좋은 것 같아요!!!!!! 포메 칭찬 포칭포칭 🐶

Copy link
Contributor

Choose a reason for hiding this comment

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

L5 - 참고 의견

저에게 정말 필요한 스크립트 ^^.. 포메 덕에 이제 그럴 일은 없겠군요 고마워요 포칭포칭 ✨

@@ -1,3 +1,5 @@
#!/bin/sh
Copy link
Contributor

Choose a reason for hiding this comment

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

L5 - 참고 의견

오 commit-msg 와 prepare-commit-msg 와는 어떤 차이가 있나요?? 실행 시점이 다를 것 같은데 현재 코드에서 실행에 차이가 생기는 건가요??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

네, 실행 시점의 차이입니다. prepare-commit-msg가 더 실행 순서가 빠릅니다.

image

현재 필요한 기능이 커밋 메시지를 "편집"하는 기능이기 때문에 커밋 메세지가 완성되기 전에 스크립트 파일이 실행되도록 commit-msg에서 prepare-commit-msg로 변경하였습니다.

echo "Error: Commit message does not contain the issue number #$ISSUE_NUMBER."
exit 1
fi
# 이슈 번호가 없으면 커밋 메시지 끝에 추가
Copy link
Contributor

Choose a reason for hiding this comment

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

L5 - 참고 의견

오 이슈 번호가 없으면 에러를 뱉는 것보다 자동으로 넣어주는 게 더 좋은 방식인 것 같아요!!! 좋네요 😎

Copy link
Contributor

Choose a reason for hiding this comment

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

L5 - 참고 의견

이슈를 매번 빼 먹어서 어 ? 왜 커밋이 안되지 ? 하는 저에게 정말 필요한 스크립트네요 .. 포칭 !!! 😊

@@ -0,0 +1,7 @@
// Test Example
Copy link
Contributor

Choose a reason for hiding this comment

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

L4 - 변경 제안

해당 테스트 코드가 커밋에 안들어가면 actions에서 오류가 발생하나요?? 테스트 코드가 없을 때 오류나는 게 아니라면 해당 파일과 디렉토리는 없어도 괜찮을 것 같아요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

네 맞아요. 로컬에서는 그냥 매칭된 파일 없으므로 나오고 끝나는데 actions에서는 에러가 발생하더라고요.... 그래서 일단 임시 테스트 파일을 넣어주었습니다.

useon
useon previously approved these changes Jul 21, 2024
Copy link
Contributor

@useon useon left a comment

Choose a reason for hiding this comment

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

이미 마루가 리뷰를 잘 달아놔서 .. 더 할 말이 없군요 .. ! 포메 덕분에 개발 생산성이 더 올라가겠어요 ~! 야호야호 고맙슴돠 🍀

echo "Error: Commit message does not contain the issue number #$ISSUE_NUMBER."
exit 1
fi
# 이슈 번호가 없으면 커밋 메시지 끝에 추가
Copy link
Contributor

Choose a reason for hiding this comment

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

L5 - 참고 의견

이슈를 매번 빼 먹어서 어 ? 왜 커밋이 안되지 ? 하는 저에게 정말 필요한 스크립트네요 .. 포칭 !!! 😊

# 원래 브랜치로 돌아가기
git checkout $current_branch

# rebase 수행
Copy link
Contributor

Choose a reason for hiding this comment

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

L5 - 참고 의견

저에게 정말 필요한 스크립트 ^^.. 포메 덕에 이제 그럴 일은 없겠군요 고마워요 포칭포칭 ✨

@novice0840 novice0840 merged commit 23c19ea into develop Jul 21, 2024
1 check passed
@novice0840 novice0840 deleted the feat/#39 branch July 21, 2024 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🫧 FE front end 🗺 infra 인프라 관련 ✅ test 테스트 관련
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[FEAT] CI / CD 환경 설정
3 participants