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

[18조 BE] 10주차 코드 리뷰 신청 #106

Merged
merged 120 commits into from
Nov 10, 2024
Merged

[18조 BE] 10주차 코드 리뷰 신청 #106

merged 120 commits into from
Nov 10, 2024

Conversation

minsu-cnu
Copy link
Contributor

@minsu-cnu minsu-cnu commented Nov 8, 2024

안녕하세요 멘토님. 18조 BE 10주차 코드 리뷰를 신청합니다! 확인하시고 답변 남겨주시면 정말 감사드리겠습니다!

추가적인 질문이 있는데 다음과 같습니다. 여기에도 답변 주시면 감사드리겠습니다!!

  1. 기능 브랜치에서 작업 후, 배포 서버에 반영하기 위해 Weekly-10 -> Develop -> Master 순으로 PR을 날려 머지시키는데 좀 불편한 것 같습니다.

    그래서 hotfix 브랜치같은 것이 떠오르긴 하는데, 이 방식으로 할 경우에는 hotfix 브랜치를 만들어서 작업 후 master와 develop에 머지하고, develop에서 weekly-10로 PR 날리고 머지시켜서 팀원들이 해당 수정사항을 각자의 기능 브랜치에 pull해서 최신화 후 작업을 이어서하면 되는 구조가 되는지 궁금합니다.

  2. CI/CD에서, GCP API에 접근해서 GCP VM에 SSH로 접속 후 인스턴스 내 배포 스크립트를 실행하는 것까지 자동화를 하려고 했는데 해결책을 못 찾아서.. 일단 지금은 SCP로 Jar 배포 파일을 서버 인스턴스 내에 전송하는 것까지만 자동화를 해두었습니다. 혹시 제가 이전에 작성했던 yml 코드를 보시고, 어떤 문제가 있었던 건지 조언 주신다면 감사드리겠습니다!

    이전에 작성했던 yml 코드 주소입니다! : https://github.com/kakao-tech-campus-2nd-step3/Team18_BE/blob/3fa159058ffea72bd1cc0434f978d897393c62c3/.github/workflows/deploy.yml

  3. 현재의 배포 스크립트에 의하면 서버 로그 파일을 배포 스크립트를 실행할 때마다 기존의 로그는 남겨두고 새로운 로그 파일을 만들어 기록을 시작하는 방식이고, 로그 파일들의 구분은 파일명에 타임스탬프를 넣어 구분하고 있습니다.
    그런데 만약 로그 파일이 훨씬 더 많아진다면, 내가 로그를 보고싶은 시점의 로그 파일이 어떤건지, 그리고 로그 파일 내에서도 원하는 시간대에 해당하는 로그 내용이 어느 위치에 있는건지 파악하기가 어려울 것 같다는 생각이 들었습니다. 현업에서는 로그 관련해서 어떻게 관리하고 유지보수하는지 궁금합니다!


안녕하세요 멘토님 저는 구인글, 이력서를 담당하는 김민지입니다!
다름이 아니라 테스트코드를 짜고 합치는 과정에서 에러가 발생했는데 아직 해결을 하지 못했습니다.
이 점 참고해주시고, 해당부분(RecruitmentServiceTest,ResumeServiceTest)부분은 코드 위주로 피드백해주시면 감사드리겠습니다 : )

test1 and others added 30 commits November 4, 2024 19:11
[유저정보, 지원] 지원서 기능 추가
[유저정보, 지원] 지원서 기능 추가
[유저정보, 지원] 지원서 기능 추가
[인증/인가] 어노테이션 누락 수정, CORS 헤더 허용 범위 최소화
[Develop] 어노테이션 누락 수정, CORS 헤더 허용 범위 최소화
[배포] 어노테이션 누락 수정, CORS 헤더 허용 범위 최소화
[인증/인가] 원활한 프론트 배포 연동 테스트를 위해 임시로 토큰 만료 기간 설정 제거
[Develop] 원활한 프론트 배포 연동 테스트를 위해 임시로 토큰 만료 기간 설정 제거
[배포] 원활한 프론트 배포 연동 테스트를 위해 임시로 토큰 만료 기간 설정 제거
[인증/인가] 프론트엔드 오리진 수정
[Develop] 프론트엔드 오리진 수정
[Master] 프론트엔드 오리진 수정
[인증/인가] 인증 헤더가 없는 API 요청에도 CORS 헤더를 반환하도록 설정
LeeTaek2T and others added 13 commits November 9, 2024 01:37
[Devleop] [유저정보] 회사response 수정
[Master] [유저정보] 회사response 수정
[인증/인가] 로그인 API가 프론트엔드의 로컬 환경과 배포 환경에서의 요청에 둘 다 대응할 수 있도록 로직 구현
[Develop] 로그인 API - 프론트 로컬 오리진, 배포 오리진 환경 대응 로직 추가
[Master] 로그인 API - 프론트 로컬 오리진, 배포 오리진 환경 대응 로직 추가
[지원] feat : 필수정보 반환값 변경
[지원] feat : 필수정보 반환값 변경
[Master] [지원] feat : 필수정보 반환값 변경
Copy link

@cheol-95 cheol-95 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 코드리뷰를 거치지 않은 코드가 배포되는것은 위험한 방식입니다. 언급하신 핫픽스의 경우 이름 그대로 긴급한 변경 사항을 운영에 바로 반영하기 위해 사용되는 방식이라 배포 프로세스가 번거롭다는 이유로 핫픽스처럼 브랜치를 관리하시면 추후 코드 관리가 어려울 수 있습니다.

지금처럼 Weekly > Develop PR리뷰를 통해 코드가 충분히 리뷰되고 빠르게 처리되면 Master로 머지하여 배포하는 프로세스를 유지하는게 좋아보이네요.


2.CI/CD
어떤식의 문제가 발생했는지 에러 메시지나 내용을 확인해야 정확한 답변이 가능할 것 같습니다.
에러 디버깅을 위한 로깅을 구축해 두는것도 좋겠네요. (set -x)

스크립트만 봤을 땐 아래 항목들이 의심스럽습니다.

  • 경로 문제
  • deploy 스크립스 실행 권한
  • 방화벽, 인스턴스 접근 설정 확인

3.배포 스크립트
현업에서를 로그를 관리하는 별도의 시스템을 구축하는편이 일반적입니다. 저의 경우, aws cloud watch로 쌓이는 JSON 포맷의 로그를 lambda를 통해 Elasticsearch로 전송하고, Kibana에서 확인하는 방식을 채택하고 있습니다. 이 방식 외에도 sentry, grafana 등 여러 대시보드 시스템을 통해 시스템을 모니터링하고 관리할 수 있으니 알아보시기 바랍니다.

  • ELK Stack: Elasticsearch, Logstash, Kibana (ELK)는 로그 수집, 저장, 시각화 및 검색 기능을 제공하여 많이 사용됩니다. 로그를 중앙으로 수집해두면, 특정 시간대나 로그 레벨에 따른 필터링 및 검색이 용이합니다. Kibana 대시보드를 통해 로그 데이터를 쉽게 탐색하고, 서버 장애 원인을 빠르게 파악할 수 있습니다.
  • 클라우드 로그 관리 서비스: GCP의 경우 Cloud Logging을 이용하여 GCP 리소스의 로그를 중앙 관리할 수 있습니다. 로그를 실시간으로 수집하고 필터링할 수 있어 장애 시점의 로그를 특정하여 빠르게 확인할 수 있습니다. 다른 주요 클라우드 서비스도 유사한 로그 수집 및 모니터링 서비스를 제공하므로 사용 중인 인프라에 따라 적합한 서비스를 선택할 수 있습니다.

현재 시스템에 적용하기는 위에 설명한 내용들이 어렵고 시간이 부족할 수 있습니다. 따라서 당장 적용시킬만한 팁을 드리자면, 배포 주기로 로그 파일을 관리하는 방식보단 디렉토리 명을 시간단위로 지정하고 파일명을 일정 시간 단위로 구분하여 저장해두면 지금보다는 편하게 로그를 찾을 수 있어보이네요.

ex)
/logs/2023/11/10/00-01.log
/logs/2023/11/10/01-02.log

4.테스트코드 관련
명확하게 어느 지점에서 에러가 났다던가, 혹은 에러 메시지를 공유해주셔야 확인이 가능합니다. 일단 단순히 코드를 봤을 때 확인이 가능한 지점만 리뷰를 남겨보겠습니다.

.github/workflows/deploy.yml Show resolved Hide resolved
.github/workflows/deploy.yml Show resolved Hide resolved
@@ -42,6 +44,14 @@ public ResponseEntity<Void> createApplicationForm(
return ResponseEntity.created(location).build();
}

@Operation(summary = "특정 지원서 조회")
@GetMapping("/form/{applyId}")

Choose a reason for hiding this comment

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

Suggested change
@GetMapping("/form/{applyId}")
@GetMapping("/{applyId}/form")

코드만 봤을 땐 이것이 더 어울려 보입니다.
자세한 도메인은 멘티님들이 더 잘 알고계시니 참고만 해주세요.

@@ -1,6 +1,6 @@
package team18.team18_be.apply.dto.response;

public record MandatoryResponse(boolean resumeExistence, boolean visaExistence,
boolean foreignerIdNumberExistence) {
boolean foreignerIdNumberExistence,boolean signExistence) {

Choose a reason for hiding this comment

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

컨벤션에 조금 더 신경써주세요.
IDE 설정을 통해 자동으로 컨벤션을 맞추는 것도 좋은 방법입니다.

Suggested change
boolean foreignerIdNumberExistence,boolean signExistence) {
boolean foreignerIdNumberExistence, boolean signExistence) {

Comment on lines +91 to +93
List<Company> comapnys = new ArrayList<>();
comapnys.add(company);
when(companyRepository.findByUser(employer)).thenReturn(Optional.of(comapnys));

Choose a reason for hiding this comment

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

Suggested change
List<Company> comapnys = new ArrayList<>();
comapnys.add(company);
when(companyRepository.findByUser(employer)).thenReturn(Optional.of(comapnys));
List<Company> companies = new ArrayList<>();
companies.add(company);
when(companyRepository.findByUser(employer)).thenReturn(Optional.of(companies));

Comment on lines +76 to +85
resumeService.saveResume(
new ResumeRequest("김민지",
"서울특별시 강남구 테헤란로 123",
"010-1234-5678",
"3년 이상의 백엔드 개발 경력",
"한국어 능력 상",
"저는 효율적인 시스템을 구축하고, 사용자의 경험을 향상시키는 것을 목표로 하는 백엔드 개발자입니다. 다양한 기술 스택을 활용하여 문제를 해결하며, 팀워크와 협업을 중요시합니다.")
, user
);
authRepository.save(user);

Choose a reason for hiding this comment

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

user먼저 저장하지 않고 resume를 저장할 수 있나요?

Comment on lines +48 to +53
recruitmentService.saveRecruitment(
new RecruitmentRequest("맛있는 밥상에서 아르바이트를 모집합니다.", "50명 이상의 직원", "서울, 대한민국",
70000000L, "정규직", "주 5일", "상근", "오전 10시부터 오후 7시까지",
"경력 5년 이상", "한식 레스토랑", "요리 관련 전공 또는 경력",
"팀워크 및 리더십 경험", "박정호", "맛있는 한상", 1L));
Recruitment recruitment = recruitmentRepository.findById(1L).get();

Choose a reason for hiding this comment

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

find메서드를 모킹하는것이 아니라면 무조건 id = 1 인 데이터가 있다고 장담할 수 없습니다. CI/CD 과정에서는 하나의 로컬 DB를 통해 병렬로 테스트를 실행할 테니까요.
saveRecruitment 메서드에서 create 된 객체의 id를 return하고, findById의 파라미터로 동적으로 넣는게 좋아보입니다.

@Test
@Transactional
@DisplayName("구인글 저장")
public void saveRecruitmentTest() throws JsonProcessingException {

Choose a reason for hiding this comment

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

테스트 전체적으로 ID를 1L 로 하드코딩한 부분을 동적으로 할당할 수 있도록 수정해보세요.

@minsu-cnu
Copy link
Contributor Author

minsu-cnu commented Nov 10, 2024

마지막 코드 리뷰까지 양질의 피드백을 많이 주셔서 감사합니다! 특히 git flow 및 배포 관련 조언 주신 것이 많은 도움이 되었습니다. 그 외 말씀해 주신 부분들도 모두 적극적으로 반영하겠습니다. 그동안 정말 고생 많으셨습니다. 감사합니다!

@cheol-95
Copy link

cheol-95 commented Nov 10, 2024

안녕하세요, 18조 백엔드 팀원 여러분.

지난 2달간 프로젝트 기획부터 디자인, 개발까지 진행하시느라 정말 수고 많으셨습니다. 하나의 프로젝트를 팀원들과 함께 시작해서 완성해 나가는 과정에서 겪은 도전과 노력이 여러분들의 성장에 큰 도움이 되었으리라 생각해요. 여러분과 함께 코드 리뷰를 하고 피드백을 주고받으면서 저 또한 학부 시절이 떠오르기도 하고, 그때의 열정과 노력이 다시금 생각나서 참 즐거웠습니다. 여러분들의 수준 높은 질문에 답변하며 저도 함께 성장할 수 있었습니다.

이번 프로젝트에서 쌓은 경험과 배움이 앞으로 개발자로서 성장하는 데 있어 큰 밑거름이 되기를 바랍니다. 끝까지 열심히 해준 것에 대해 진심으로 감사드리며, 앞으로도 계속해서 빛나는 개발자로 성장해 나가시길 응원할게요.

정말 고생 많으셨습니다!
강철 드림.

@cheol-95 cheol-95 merged commit 68619c9 into Review Nov 10, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants