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

PR 코드 분석 시 suggestion 태그를 위한 로직 추가 #744

Closed
wants to merge 30 commits into from

Conversation

JJ503
Copy link
Member

@JJ503 JJ503 commented Nov 9, 2023

📄 작업 내용 요약

PR 코드 분석 시 suggestion 태그를 위한 로직 추가
자세한 내용은 이슈 내용 확인해주세요!

🙋🏻 리뷰 시 주의 깊게 확인해야 하는 코드

author-info라는 jobs가 동일되는 내용이라 분리해 봤는데 잘되길 바랍니다.
제안 pr 관련 workflow의 경우 synchronize는 작동하지 않도록 했기에, 현재 pr에서는 따로 체크가 되지 않는 것이 맞습니다.
synchronize도 체크 표시는 되고(하는 건 없음, 정말 체크만) 슬랙 메시지만 안 오길 바라신다면 의견 말씀해주시면 감사하겠습니다.

그리고 몇 가지 질문이 있습니다. 답변해주시면 감사하겠습니다!

  1. 이제 직접 멘션을 해줘야 하다보니 pr 리뷰 요청에서도 author를 멘션하도록 했는데 어떤가요?
  2. 조건이 더러운 것 보다는 workflow를 아예 나누는 것이 더 좋다고 판단해 그렇게 진행했는데 어떨까요?
  3. 제안 pr의 경우 리뷰어들이 바로 멘션에 걸리는 것이 더 좋을까요?

📎 Issue 번호

@JJ503 JJ503 added environment 환경설정 변경 시 backend 백엔드와 관련된 이슈나 PR에 사용 suggestion 코드 변경 사항이 있는 경우 팀원들에게 제안하기 위함 labels Nov 9, 2023
@JJ503 JJ503 added this to the 레벨 5 milestone Nov 9, 2023
@JJ503 JJ503 requested review from apptie, swonny and kwonyj1022 November 9, 2023 04:48
@JJ503 JJ503 self-assigned this Nov 9, 2023
Copy link
Collaborator

@apptie apptie left a comment

Choose a reason for hiding this comment

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

칭찬

와 개쩌러요!!!!!!!!!!! github action 잘하시네요 인프라 가시죠

질문에 대한 답은.. step가 더 나을 것 같습니다 굳이 author-info를 job으로 볼 필요는 없다고 생각합니다

질문

제안하는 PR의 경우 리뷰어에게 바로 멘션이 걸리는 건 어케 생각하시는지 궁금합니다
jacoco도 안하고 테스트 통과나 애플리케이션 실행 등 이런 부분을 제안 단계에서는 신경쓰지 않아도 된다고 생각해서 그렇습니다

@JJ503 JJ503 added backend 백엔드와 관련된 이슈나 PR에 사용 and removed suggestion 코드 변경 사항이 있는 경우 팀원들에게 제안하기 위함 backend 백엔드와 관련된 이슈나 PR에 사용 labels Nov 9, 2023
@JJ503 JJ503 added the suggestion 코드 변경 사항이 있는 경우 팀원들에게 제안하기 위함 label Nov 9, 2023
@JJ503 JJ503 removed the suggestion 코드 변경 사항이 있는 경우 팀원들에게 제안하기 위함 label Nov 9, 2023
@JJ503 JJ503 added the suggestion 코드 변경 사항이 있는 경우 팀원들에게 제안하기 위함 label Nov 9, 2023
@JJ503 JJ503 removed the suggestion 코드 변경 사항이 있는 경우 팀원들에게 제안하기 위함 label Nov 9, 2023
@JJ503 JJ503 added the suggestion 코드 변경 사항이 있는 경우 팀원들에게 제안하기 위함 label Nov 9, 2023
@JJ503 JJ503 removed the suggestion 코드 변경 사항이 있는 경우 팀원들에게 제안하기 위함 label Nov 9, 2023
@JJ503 JJ503 added the suggestion 코드 변경 사항이 있는 경우 팀원들에게 제안하기 위함 label Nov 9, 2023
Copy link
Collaborator

@kwonyj1022 kwonyj1022 left a comment

Choose a reason for hiding this comment

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

  1. 이제 직접 멘션을 해줘야 하다보니 pr 리뷰 요청에서도 author를 멘션하도록 했는데 어떤가요?

좋습니다.

  1. 조건이 더러운 것 보다는 workflow를 아예 나누는 것이 더 좋다고 판단해 그렇게 진행했는데 어떨까요?

좋습니다.

  1. 제안 pr의 경우 리뷰어들이 바로 멘션에 걸리는 것이 더 좋을까요?

어떻게 돼도 상관없지만 바로 멘션 걸리면 좀 더 편할 것 같습니다. 기존에 보통 자코코 때문에 수정 후 다시 요청하는 상황이 생겼는데 제안 pr은 자코코를 신경 쓰지 않아도 되니까 바로 멘션 걸려도 괜찮을 것 같습니다.

질문

제안 pr은 approve 2개 이상 받아도 머지 못하게 하는 기능을 추가하는 건 어떤가요?

@JJ503
Copy link
Member Author

JJ503 commented Nov 22, 2023

@apptie

step가 더 나을 것 같습니다 굳이 author-info를 job으로 볼 필요는 없다고 생각합니다

(당시 지토는 job이 분리된 코드를 보셨을 텐데, workflow를 분리하는 방식으로 수정했고 그에 따른 의견으로 작성했습니다.)
저는 job도 아닌 아예 다른 workflow로 봤습니다. suggestion의 경우 build도 필요가 없고, 오로지 리뷰어들에게 알리기 위한 용도로 사용됩니다. 그에 반해 suggestion이 아닌 경우에는, 처음에만 알려줄 뿐 build가 목적인 workflow라고 봤기 때문입니다.
그래서 suggestion에서는 저희가 기존에 사용하던 workflow는 아예 실행조차 되지 않아, 하나의 workflow만 실행된 결과를 확인하실 수 있습니다.
그리고 사실 처음 이를 생각하게 된 건 하나의 workflow 그리고 job으로 실행했을 때, github action의 조건이 위 step의 success, fail, skip 여부에 달려있어 다른 사람들이 조건만 보고 판단하기가 어렵다고 생각했기 때문입니다.


@kwonyj1022

제안 pr은 approve 2개 이상 받아도 머지 못하게 하는 기능을 추가하는 건 어떤가요?

좋은 의견이네요!
어차피 build는 수행하지 않을 것이고, 해당 workflow는 리뷰어에게 알리는 용도일 뿐이기에 무조건 실패해도 이 부분이 신경 쓰이지는 않을 것 같아요!


@공통
두 분 모두 동일하게 멘션이 되었으면 좋겠다고 해주셔서 과반수라고 생각해, 진행하도록 하겠습니다!

Copy link
Collaborator

@swonny swonny left a comment

Choose a reason for hiding this comment

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

제이미 고생 많으셨습니다 ❤️

@JJ503
Copy link
Member Author

JJ503 commented Dec 8, 2023

제안 태그가 있는 pr의 경우 강제로 실패하도록 수정했습니다.
이제 프로젝트가 자유로운 형태가 되어 채널을 나가는 인원이 발생할 수도 있을 것이라 판단해, @channel을 통해 리뷰어를 호출하는 형태로 변경했습니다.

해당 제안 PR은 Close 후 실제 적용 PR로 다시 리뷰 요청 드리도록 하겠습니다.

@JJ503 JJ503 closed this Dec 8, 2023
@JJ503 JJ503 deleted the suggestion/env/740 branch December 16, 2023 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend 백엔드와 관련된 이슈나 PR에 사용 environment 환경설정 변경 시 suggestion 코드 변경 사항이 있는 경우 팀원들에게 제안하기 위함
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants