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

flyway ci 검증 추가 #730

Merged
merged 72 commits into from
Nov 3, 2023
Merged

flyway ci 검증 추가 #730

merged 72 commits into from
Nov 3, 2023

Conversation

JJ503
Copy link
Member

@JJ503 JJ503 commented Nov 1, 2023

📄 작업 내용 요약

  • flyway ci 검증 추가
  • 기존 ci 검증의 슬랙 메시지에 대한 정책 변경 (리뷰어 직접 멘션으로)

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

  • backend/ddang/src/main/resources/db/migration 경로에 **.sql 파일이 생성 혹은 수정 사항이 일어나야 flyway에 대한 ci 검증이 수행되도록 했습니다.

  • 다만 주의해야 할 점은, 각 버전별 유효성만 검사할 뿐 JPA entity와의 검증을 불가능합니다. 그러니 엔티티 작성 시 최대한 실수를 하지 않도록 주의해야 합니다.

  • flyway ci 검사 성공
    image

  • flyway ci 검사 실패
    image

📎 Issue 번호

@JJ503 JJ503 added environment 환경설정 변경 시 backend 백엔드와 관련된 이슈나 PR에 사용 labels Nov 1, 2023
@JJ503 JJ503 added this to the 레벨 5 milestone Nov 1, 2023
@JJ503 JJ503 requested review from apptie and swonny November 1, 2023 06:42
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.

질문 & 필수

workflow 파일에는 문제가 없는데 기존 workflow나 슬랙 메세지 관련해서 질문 드리고 싶은 부분이 있어 RC 날립니다

지금은 flyway workflow와 test workflow가 별도로 비동기적으로 수행되고 있는 것으로 보입니다
이와 관련해 다음과 같은 질문이 있습니다


  1. flyway workflow와 test workflow가 따로따로 동작하게 한 이유가 궁금합니다. workflow에 대한 트기러를 통해 분기 처리가 가능해보이기도 하고, flyway workflow가 실패했다면 test workflow를 실행시킬 필요가 없어 보인다는 생각이 들었습니다
  2. flyway workflow에 대한 슬랙 메세지는 필요가 없는지 궁금합니다. flyway workflow가 실패하고 test workflow가 성공했다면 슬랙 메세지는 성공했다고 할텐데, 이러한 메세지가 리뷰어한테 혼란을 줄 수 있다고 생각합니다. 특히 성공 시 리뷰어에게 멘션을 자동으로 걸어버려서 더욱 헷갈릴 수 있다고 생각합니다. flyway workflow와 test workflow에 대한 슬랙 메세지 정책을 한 번 고려해볼 필요가 있다고 생각합니다.

요 부분은 메리의 의견도 필요한 것 같습니다 그렇습니다
고생하셨습니다

@apptie
Copy link
Collaborator

apptie commented Nov 2, 2023

그리고 커밋 메세지 깨진거 신경쓰이는데 어케 못하나요

@JJ503
Copy link
Member Author

JJ503 commented Nov 2, 2023

동시에 진행하는 것이 효율상 좋을 것이라 판단해 분리했습니다.
다만, 지토의 의견대로 backend_pr_decorator.yml에 대한 알람만 가거나, 둘 다 각각 슬랙에 알람이 간다면 이는 리뷰어들에게 혼란을 가중시킬 것 같습니다.
그래서 차라리 pr 슬랙 메시지 정책을 변경하는 것이 어떨까 싶습니다.
리뷰어들을 멘션하는 것을 제거하고 작성자만 멘션을 하는 것은 어떤가요?
작성자가 슬랙에 pr 메시지가 뜬 후 리뷰를 받을 준비가 된다면 그때 직접 멘션 해줘도 괜찮을 것 같습니다.
현재도 종종 그런 식으로 진행하는 것이 많은 것 같아 pr을 매번 알람을 주지 않는 이상 현재 메시지 정책은 크게 의미가 없을 것 같다는 생각이 들기도 합니다.

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.

두 분 엄청나게 고생이 많으셨군요..!👏🏻
페어룸이랑 회의실에서 열심히 하시더니 멋집니다

암튼 지토가 남겨주신 의견에 대해 제가 이해한건

flyway workflow랑 test workflow가 비동기로 동작하기 때문에 두 작업이 따로 동작하게 되고, 리뷰어가 동일한 PR에 대한 알림을 두 번 받게되어 혼란을 야기할 수 있다

인 것 같습니다. 맞다면, 지토 의견에 동의합니다. 둘 중 하나라도 실패한다면 알림이 가지 않는 것이 자연스러울 것 같아요! 이전에 저희가 한 번 올린 PR에 대한 push가 있을 때마다 알림이 오면 헷갈렸던 것처럼 동일한 PR에 대한 알림은 한 번만 가도록 하고, 이후에는 리뷰이가 멘션을 통해 알리는 것이 좋을 것 같다는 의견입니다.

1번과 2번이 연결되는 내용이라고 이해했는데, 제이미가 말씀해주신 것처럼 리뷰이한테만 알림이 가고, 리뷰이가 PR에 대한 판단을 한 뒤 리뷰어를 멘션하는 방법도 좋은 방법이라고 생각합니다!

@JJ503
Copy link
Member Author

JJ503 commented Nov 3, 2023

말씀해 주신 결과에 따라 pr 메시지는 기존 검증인 backend_pr_decorator.yml에서만 발생하는 것으로 하겠습니다.
다만, 리뷰어 멘션 거는 것만 제거하도록 하겠습니다!
그리고 추가 의견이었던 슬랙 pr 알림 메시지에 직접 멘션을 해줘야 한다는 의미의 문장도 추가해주었습니다.

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.

이상이? 없는 것 같아? approve 하겠습니다
나중에 테스트는 해봐야겠네요

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 JJ503 merged commit f868d3a into develop-be Nov 3, 2023
1 check passed
@JJ503 JJ503 deleted the env/728 branch November 3, 2023 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend 백엔드와 관련된 이슈나 PR에 사용 environment 환경설정 변경 시
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants