Conversation
Added commands for code style checks and Docker usage.
related_to : #7 ## 📝 작업 내용 CD 추가 과정에서 있었던 피드백을 반영했습니다. (PR #8 ) ## ✅ 변경 사항 - [x] .env.* 추가 - [x] @requiredargsconstructor사용해서 DI로 수정 - [x] 기타 description과 api version 제거 ## 📷 스크린샷 (선택) ## 💬 리뷰어에게 아 로컬에서 PR을 안날리고 다시 했네요... 변경 사항 추가했습니다
Summary of ChangesHello @ParkJiYeoung8297, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 애플리케이션의 지속적인 배포(CD) 환경을 구축하고, 전반적인 인프라를 강화하는 데 중점을 둡니다. JWT 기반의 강력한 보안 시스템을 도입하여 사용자 인증 및 권한 관리를 개선하고, Swagger를 통해 API 문서화를 자동화하여 개발 편의성을 높였습니다. 또한, Checkstyle을 도입하여 코드 품질을 일관되게 유지하며, 공통 엔티티 및 응답 형식을 정의하여 코드의 재사용성과 유지보수성을 향상시켰습니다. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
이 PR은 자동 배포를 위한 CD 워크플로우와 관련 설정을 추가합니다. 전반적으로 Spring Security, JWT, Swagger, 표준 응답 구조 등 좋은 기반을 마련했습니다. 하지만 몇 가지 수정이 필요한 부분이 있습니다. build.gradle 파일에 중복된 의존성이 있고, SecurityConfig의 경로 설정이 컨트롤러와 일치하지 않아 API가 정상적으로 동작하지 않을 수 있습니다. 또한, application-test.yml 파일에 데이터베이스 접속 정보를 하드코딩한 것은 심각한 보안 문제이므로 반드시 수정해야 합니다. 자세한 내용은 각 파일에 대한 개별 코멘트를 참고해주세요.
| username: admin | ||
| password: secret |
There was a problem hiding this comment.
| private static final String[] PERMIT_ALL_URL_ARRAY = { | ||
| "/api/v1/test", | ||
| "/v3/api-docs/**", "/swagger-ui/**", "/swagger-resources/**", "/swagger-ui.html" | ||
| }; | ||
|
|
||
| private static final String[] REQUEST_AUTHENTICATED_ARRAY = { | ||
| "/api/v1/test-auth" | ||
| }; |
There was a problem hiding this comment.
Security 설정에 정의된 URL 경로(PERMIT_ALL_URL_ARRAY, REQUEST_AUTHENTICATED_ARRAY)가 TestController에 설정된 경로와 일치하지 않습니다. SecurityConfig에는 /api/v1/test로 되어 있지만, 컨트롤러에는 /api/test로 되어 있어 v1이 빠져있습니다. 이로 인해 API 호출 시 403 Forbidden 오류가 발생할 수 있습니다. 경로를 일치시켜야 합니다.
| private static final String[] PERMIT_ALL_URL_ARRAY = { | |
| "/api/v1/test", | |
| "/v3/api-docs/**", "/swagger-ui/**", "/swagger-resources/**", "/swagger-ui.html" | |
| }; | |
| private static final String[] REQUEST_AUTHENTICATED_ARRAY = { | |
| "/api/v1/test-auth" | |
| }; | |
| private static final String[] PERMIT_ALL_URL_ARRAY = { | |
| "/api/test", | |
| "/v3/api-docs/**", "/swagger-ui/**", "/swagger-resources/**", "/swagger-ui.html" | |
| }; | |
| private static final String[] REQUEST_AUTHENTICATED_ARRAY = { | |
| "/api/test-auth" | |
| }; |
| testImplementation 'org.springframework.boot:spring-boot-starter-test' | ||
| testRuntimeOnly 'org.junit.platform:junit-platform-launcher' | ||
|
|
||
| // Test dependencies | ||
| testRuntimeOnly 'com.h2database:h2' | ||
| testRuntimeOnly 'org.junit.platform:junit-platform-launcher' | ||
| testImplementation 'org.springframework.boot:spring-boot-starter-test' | ||
| testImplementation 'org.springframework.security:spring-security-test' | ||
| testRuntimeOnly 'org.junit.platform:junit-platform-launcher' |
There was a problem hiding this comment.
build.gradle 파일에 중복된 테스트 의존성이 있습니다. testImplementation 'org.springframework.boot:spring-boot-starter-test'와 testRuntimeOnly 'org.junit.platform:junit-platform-launcher'가 여러 번 선언되었습니다. 중복을 제거하여 dependencies 블록을 더 깔끔하게 유지하는 것이 좋습니다.
testImplementation 'org.springframework.boot:spring-boot-starter-test'
testRuntimeOnly 'org.junit.platform:junit-platform-launcher'
// Test dependencies
testRuntimeOnly 'com.h2database:h2'
testImplementation 'org.springframework.security:spring-security-test'
| public void softDelete(Integer deletedBy, LocalDateTime deletedAt) { | ||
| this.isDeleted = true; | ||
| this.deletedAt = deletedAt; | ||
| } |
There was a problem hiding this comment.
softDelete 메소드의 deletedBy 파라미터가 사용되지 않고 있습니다. 삭제한 사용자를 기록하는 필드가 없다면 이 파라미터를 제거하는 것이 좋습니다. 만약 추후에 사용할 계획이라면, 해당 필드를 추가하고 값을 할당하는 로직을 구현해야 합니다.
| public void softDelete(Integer deletedBy, LocalDateTime deletedAt) { | |
| this.isDeleted = true; | |
| this.deletedAt = deletedAt; | |
| } | |
| public void softDelete(LocalDateTime deletedAt) { | |
| this.isDeleted = true; | |
| this.deletedAt = deletedAt; | |
| } |
| public CorsConfigurationSource corsConfigurationSource() { | ||
|
|
||
| CorsConfiguration configuration = new CorsConfiguration(); | ||
| configuration.setAllowedOrigins(List.of(allowedOrigin, "http://localhost:8080", swaggerUrl)); |
There was a problem hiding this comment.
CORS 설정에서 허용할 오리진으로 http://localhost:8080이 하드코딩되어 있습니다. 로컬 개발 환경을 위한 것이겠지만, 이 또한 application.yml 파일에서 설정값으로 관리하는 것이 더 유연하고 좋은 방법입니다.
| configuration.setAllowedOrigins(List.of(allowedOrigin, "http://localhost:8080", swaggerUrl)); | |
| configuration.setAllowedOrigins(List.of(allowedOrigin, swaggerUrl)); |
| .claim("role", role) | ||
| .issuedAt(new Date(now)) | ||
| .expiration(new Date(now + expireMillis)) | ||
| .signWith(secretKey, SignatureAlgorithm.HS256) |
There was a problem hiding this comment.
| "잘못된 요청입니다."), | ||
|
|
||
| INVALID_DATA(HttpStatus.BAD_REQUEST, | ||
| "COMMON4001_2", |
|
|
||
| jpa: | ||
| hibernate: | ||
| ddl-auto: none |
Yoonchulchung
left a comment
There was a problem hiding this comment.
제가 이전에 올린 코드에 조금 오류가 있습니다. main으로 merge 하시기 전에 수정 부탁드립니다.
.github/workflows/cd-prod.yml
Outdated
| context: . | ||
| push: true | ||
| tags: | | ||
| ${{ secrets.DOCKERHUB_USERNAME }}/spot-backend:prod |
There was a problem hiding this comment.
제가 이전 프로젝트에 사용했던 flow가 올라온 것 같습니다. 확인 부탁드립니다. 도커 허브 주소는 노션에 남겨주시길 부탁드립니다.
There was a problem hiding this comment.
프로젝트 관련 설정이 모두 spot으로 되어있어서 그거에 맞춰서 했습니다. 이걸 realmatch로 수정하겠습니다.
| - name: Deploy | ||
| uses: appleboy/ssh-action@v1.0.3 | ||
| with: | ||
| host: ${{ secrets.PROD_SERVER_IP }} |
There was a problem hiding this comment.
PROD_SERVER_IP는 가비아 IP가 맞나요?
There was a problem hiding this comment.
네네 노션에 올려주신 IP로 사용했습니다.
| public class SecurityConfig { | ||
| private final JwtAuthenticationFilter jwtAuthenticationFilter; | ||
|
|
||
| private static final String[] PERMIT_ALL_URL_ARRAY = { |
There was a problem hiding this comment.
하드코딩 되어 있는 API주소를 .env에서 관리될 수 있도록 변경해주세요
There was a problem hiding this comment.
url을 .env로 관리하면, api 개발할 때마다 추가해줘야하고 가독성도 떨어져서 개발이 불편할 거 같은데,
따로 관리하려고 하시는 이유가 있을까요?
There was a problem hiding this comment.
API 개발을 할때마다 .env는 항상 가지고 있어야 실행 가능하지 않나요? 따로 관리하는 이유는 인증을 개발하지 않는 개발자들도 쉽게 모든 사용자가 접근할 수 있는 API를 쉽게 추가할 수 있도록 구성하기 위해서 .env로 따로 관리하는 것이 좋을 것 같다고 생각했습니다. 또한 개발 단계에서는 고려하지 않아도 괜찮을 수 있지만, 배포를 고려하기도 하다 보니까 API가 노출되어 있으면 쉽게 공격당할 수 있지 않아요?
There was a problem hiding this comment.
.env로 관리할 경우 아래와 같은 과정이 반복되어서 불편할것이라고 생각했습니다.
- 로컬에서 .env 추가
- 서버에 접속해서 .env 추가
- 노션에서 .env 업데이트
- 다른 팀원들이 업데이트 된 .env 가져가기
.env가 깃으로 관리되는 것이 아니다보니 공유도 실시간으로 어려울거 같았습니다.
그리고 API는 브라우저에 공개되어 있는 값이라 url을 숨기는 것 보다는,
api요청이 들어왔을 때 인증을 거치도록 설정하면 보안상 괜찮을 것 같습니다.
어떻게 생각하시는지 의견 부탁드립니다!🙂
There was a problem hiding this comment.
관리가 불편한 것은 인프라로도 해결할 수 있는 부분이 있지만, 아직 거기까지는 하기는 어렵기 때문에 이 형태로 진행해도 괜찮을 것 같습니다. 👍
build.gradle
Outdated
| implementation 'org.springdoc:springdoc-openapi-starter-webmvc-ui:2.8.9' | ||
|
|
||
| // postgresql (CI test 용) | ||
| runtimeOnly 'org.postgresql:postgresql' |
There was a problem hiding this comment.
postgresql은 지워주시면 감사하겠습니다.
.github/workflows/pr-check.yml
Outdated
|
|
||
| services: | ||
| postgres: | ||
| image: postgres:16 |
There was a problem hiding this comment.
이 부분 mySQL에서 동작되도록 변경해주시면 감사하겠습니다.
.github/workflows/pr-check.yml
Outdated
| postgres: | ||
| image: postgres:16 | ||
| env: | ||
| POSTGRES_DB: myapp_db |
There was a problem hiding this comment.
DB 정보는 secrets에서 관리될 수 있도록 해주시면 감사하겠습니다.
There was a problem hiding this comment.
윤철님이 테스트용으로 가짜 값 넣어두신줄 알았습니다. mysql로 바꾸면서 시크릿으로 관리하겠습니다.
<!-- ## PR 제목 컨벤션 [TYPE] 설명 (#이슈번호) 예시: - [FEAT] 회원가입 API 구현 (#14) - [FIX] 이미지 업로드 시 NPE 수정 (#23) - [REFACTOR] 토큰 로직 분리 (#8) - [DOCS] ERD 스키마 업데이트 (#6) - [CHORE] CI/CD 파이프라인 추가 (#3) - [RELEASE] v1.0.0 배포 (#30) TYPE: FEAT, FIX, DOCS, REFACTOR, TEST, CHORE, RENAME, REMOVE, RELEASE --> ## Summary <!-- 변경 사항을 간단히 설명해주세요 --> ## Changes <!-- 변경된 내용을 목록으로 작성해주세요 --> ## Type of Change <!-- 해당하는 항목에 x 표시해주세요 --> - [ ] Bug fix (기존 기능에 영향을 주지 않는 버그 수정) - [x] New feature (기존 기능에 영향을 주지 않는 새로운 기능 추가) - [ ] Breaking change (기존 기능에 영향을 주는 수정) - [ ] Refactoring (기능 변경 없는 코드 개선) - [ ] Documentation (문서 수정) - [ ] Chore (빌드, 설정 등 기타 변경) - [ ] Release (develop → main 배포) ## Related Issues related_to : #11 ## 참고 사항 <!-- 리뷰어가 알아야 할 추가 정보가 있다면 작성해주세요 -->
related_to : #7
📝 작업 내용
자동 배포를 위한 CD 추가
✅ 변경 사항
📷 스크린샷 (선택)
💬 리뷰어에게
Main 브랜치에 pr이 올라가야 작동하기 때문에, dev 브랜치 그대로 PR 날립니다.
환경변수는 서버에 직접 .env 파일로 올려두었습니다. application-prod.yml을 기준으로 작동합니다.
(→ SPRING_PROFILES_ACTIVE=prod)