Replies: 2 comments 1 reply
-
제가 여러모로 너무 완벽한 PR 단위의 강박을 느꼈던 것 같아요. 그런 관점에서 API보다 더 작은 단위의 PR을 전하는 것도 좋을 것 같아요.
그러네요 충분히 저희 백엔드 4명의 팀원이 업무 진행 상황을 머리속에 그리며 컨트롤 할 수 있는 부분인데 너무 구현되지 못할까 걱정하는 것보다는 저희가 정한 컨벤션을 지키면서 작은 단위로 활발한 리뷰 문화를 유지하는 것이 더욱 중요해보입니다.
저는 이 부분은 동의를 하는 부분도 있지만, 약간 같이 정하고 싶은 부분이 있어요. 제 생각엔 Dev 서버도 엄연히 안드로이드라는 내부팀의 클라이언트 들이 사용을 하고 있는데, 만약 터지는 기능이 있다면 그것을 머지시키고 무기한으로 유지시키는것은 옳지 않다고 생각해요. 그렇다면 제 생각엔 3가지 관점이 존재할 수 있다고 생각합니다.
지금 생각나는 부분은 이정도가 있는데, 이에 대해서 어떻게 생각하시는지 궁금합니다! |
Beta Was this translation helpful? Give feedback.
-
|
전 1번 관점이 제일 좋은 것 같습니다. 2번은 사실 이해를 제대로 못했고, 3번은 현실적으로 항상 일어나기가 쉽지 않을 것 같아요 |
Beta Was this translation helpful? Give feedback.
-
|
뒤늦게 토론에 참여하게 되었네요.
이 부분에 대해서 동감하고, 장소 검수 등록 조회 기능을 한 번에 하는 일은 다시 없도록 하겠습니닿ㅎ
여러가지 기능(A+B)이 한번에 포함된다면, 위의 코코닥 말대로 A기능은 완벽한데 B기능은 문제가 있을 경우를 고려해 분리하는 것이 맞겠네요 |
Beta Was this translation helpful? Give feedback.
-
이점에 대해서는 개인적인 의견이 있습니다. 일단 전반적인 맥락을 말씀드리고, 루카와 반대되는 의견을 주장해볼게요.
첫번째!
아마 루카가 이런 생각을 하신 이유는, 아무일도 하지 않는 임시방편의 코드 때문이겠죠?!
결론부터 말씀드리자면, 해당 API 구현 내용은 전혀 생각하지 않고 작업했습니다. 이 PR을 작업할 시점에서 삭제 API 의 원래 담당자는 제가 아닌 이레였는데요, 이 PR을 작업하다보니 삭제에 대한 로직이 필수로 필요했었습니다.
저도 이 PR 이 나올때, 등록 허가에 대한 모든 기능이 포함되어서 나왔으면 좋았겠지만, 만약 포함시켰다면 이는 하나의 일을 하는 PR 이 아닙니다. (물론 ManagerAuthInterceptor 를 이 PR에서 고쳐서 할 말이 없긴 합니다만...)
API 를 뚫는 행위는 코드량으로는 적을 수 있지만, 의미적으로는 큰 태스크라고 생각합니다. 한 PR 안에서 등록 허가 API 와 삭제 API 로직 구현을 모두 하고 싶지는 않았습니다. 또한, 그것을 나누는 것이 코드 리뷰를 하는 사람 입장에서도 따라가기 수월할 거라고 생각했습니다.
다만, 제가 이 PR 코멘트에 전반적인 맥락을 담지 않았었기 때문에(임시방편의 코드가 왜 있는건지 등등).. 코드리뷰하는 사람에게 혼란을 줄 수도 있었겠다고 생각되네요. 이 부분은 반성하겠습니다 ㅠㅠ
말씀드리고 싶었던 점은, 당시 담당자였던 이레와 미리 의논하면서, 삭제에 대한 메서드 스펙을 미리 맞추어놓았고, 해당 메서드가 어떤 서비스에서 가질지도 미리 맞추어놓다보니, 저는 해당 의논 내용을 토대로 임시방편의 코드를 넣게 되었습니다. (이렇게 해야 merge conflict 범위가 최소화될 것이라 생각했습니다)
두번째!
말씀하신대로, 정말 극단적으로 생각한다면 거절 기능 PR은 closed 되고, 이에 대한 기능이 등록 허가 PR 내부로 흡수될 수도 있다고는 생각합니다.
다만 이것은 저희 팀의 규모가 충분히 작다는 점, 그리고 거절 기능에 대한 작업량이 많지 않았다는 점들이 복합적으로 맞물려서 이와 같은 생각이 드는 것 같아요.
만약 저희 팀의 규모가 더 커서 업무 부서가 다 찢어져있거나, 거절 기능에 대한 작업량이 많아서 한 PR에 합쳐질 규모가 아니라면.. 이때는 서로 깊은 연관성이 있는 PR 이더라도 분리시켜서 작업하는 것이 맞다고 생각합니다.
저는 보다 일관적이고 일반적인 방법론을 적용하는 것을 선호하는데요. 팀의 규모 크던 작던, 그 PR의 작업량이 많던 적던, 그것에 신경쓰지 않고 적용할 수 있는 방법론은
PR의 연관성이 짙던 얕던, 의미적으로 혹은 작업량적으로 큰 작업들은 한개이상의 PR로 적절히 분리되어 찢어져야한다라고 생각합니다.세번째!
전 이 부분에 동의하지 않아요 (루카형이 싫어서 그런게 아닙니다 알죠?)
저희 팀이 모두 동의했었던,
PR을 작게 유지하자!룰이 도입된 순간부터, 항상 PR의 작업 커밋들의 모음이 하나의 완전한 기능을 의미한다는 것을 보장할 순 없습니다.중요한 점은, 현재 이 PR이 merge 되고 싶어하는 브랜치는 운영 브랜치가 아닌 개발 브랜치라는 점이라고 생각해요. 운영 브랜치에 나가는 코드들은 모두 완전한 기능을 갖춘 상태로 나가야한다 생각하지만, 개발 브랜치는 그렇지 않습니다. 저는 개발 브랜치에 있는 코드들이 모두 완전한 기능을 하진 않아도 된다고 생각해요.
개발 브랜치에 올라와있는 어떤 API 코드는 실제로 동작하진 않지만, 그 기능을 완수하기 위해 공사중인 코드일 수도 있구요. 완전하지 않지만 PR을 작게 유지하면서 dev 브랜치에 꾸준히 merge가 되는 것이, 훗날 팀원들의 merge conflict 비용을 줄여주는 행위이고, 이것이 진짜 지속적 통합이라고 생각합니다.
이와 관련해서 이 글에 좋은 내용이 담겨있는데요!
여기서 있는 사진을 보시면 이해가 되실거라 생각합니다.
이런 관점에서, 장소 등록 허가 PR은 이번 PR 한번으로 완성이 될 수 없는 작업물이라고 생각합니다.
장소 등록 허가 API 에서 사용할 삭제 로직이 아직 완전하지 않아서, 이번 PR이 merge 된다해도 장소 등록 허가 API 기능이 동작할 순 없겠지만..
여러번에 걸쳐서 하나의 기능이 완성될 수 있다고 생각하고, 그게 팀적으로 좋은 방향이라 믿고 있습니다.
Beta Was this translation helpful? Give feedback.
All reactions