-
Notifications
You must be signed in to change notification settings - Fork 0
refactor: Integrating Configuration Files and Docker-Compose #214
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
Conversation
dnwls16071
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다.
리뷰 남겨드린 내용은 한 번 더 고민해주세요.
.env.example
Outdated
| REDIS_HOST= | ||
| REDIS_PORT= | ||
|
|
||
| # Spring Profile | ||
| SPRING_PROFILES_ACTIVE= | ||
|
|
||
| # Application Port | ||
| HTTP_PORT= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 변수들이 별도의 .env로 관리가 될 필요성이 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
별도의 관리가 불필요하여 제거 하였습니다!
| elasticsearch: | ||
| image: docker.elastic.co/elasticsearch/elasticsearch:8.11.0 | ||
| container_name: elastic | ||
| environment: | ||
| - discovery.type=single-node | ||
| - xpack.security.enabled=false | ||
| - xpack.security.http.ssl.enabled=false | ||
| - ES_JAVA_OPTS=-Xms1g -Xmx1g | ||
| ports: | ||
| - "9200:9200" | ||
| - "9300:9300" | ||
| volumes: | ||
| - ./elasticsearch/data:/usr/share/elasticsearch/data | ||
| healthcheck: | ||
| test: [ "CMD", "curl", "-f", "http://localhost:9200/_cluster/health" ] | ||
| interval: 10s | ||
| timeout: 5s | ||
| retries: 5 | ||
| start_period: 30s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ElasticSearch를 사용하는 기능을 대체하면 이 부분은 지우면 될 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네 한번에 다 수정하기에는 양이 많아서 나눠서 진행 하겠습니다.
| import org.atdev.artrip.jwt.jwt.JwtAuthenticationFilter; | ||
| import org.atdev.artrip.jwt.jwt.JwtProvider; | ||
| import org.atdev.artrip.jwt.jwt.exception.JwtExceptionFilter; | ||
| import org.atdev.artrip.jwt.JwtAuthenticationFilter; | ||
| import org.atdev.artrip.jwt.JwtProvider; | ||
| import org.atdev.artrip.jwt.exception.JwtExceptionFilter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| @Column(name = "curation_type", nullable = false) | ||
| private CurationType curationType; | ||
|
|
||
| @Builder.Default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Builder 패턴과 정적 팩토리 메서드 패턴 둘 중 어느 것이 좋은지 객체 사용 패턴을 보고 고민해주세요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
두 패턴을 같이 사용하는 경우도 있는지 궁굼합니다. 가능 하다면 댓글 달아주신 부분과 같은 현상이 있다면 정적메서드 패턴으로 사용하며, 두 패턴을 동시에 사용하지 못한다면 정적메서드 패턴으로 사용을 지향하고싶습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Builder.Default는 빌더 패턴 사용 시에만 기본값으로 적용이 됨을 말해요.
클래스 단에 빌더를 명시하면 다른 개발자가 일관성없이 아무런 객체들을 만들게 될거구요.
네이밍을 주고 싶고 명확하게한다면 정적 팩토리 메서드가 권장됩니다. 다만 정적 팩토리 메서드도 단점이 있으니 이 부분은 확인해보시면 좋을 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
큐레이션 생성시 isActive가 동일한 초기 값을 가져야하고 생성에 필요한 파라미터가 많지 않다면 정적 팩토리 메서드도 적절해 보입니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Builder.Default 관련하여 큐레이션 뿐만 아닌 다른 코드들도 확인 후 수정은 새로운 이슈 생성해서 작업 하도록하겠습니다!
dnwls16071
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yml은 일단 local, dev, prod로 분리해두되 속성 설정값은 그 때마다 맞춰서 바꿔주는 것이 어떨까요?
| servlet: | ||
| multipart: | ||
| max-file-size: 2MB | ||
| max-request-size: 10MB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 프로퍼티는 모든 환경에서 공통이기 때문에 상단에 쓰셔도 될 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
servlet 프로퍼티는 상단으로 수정하였습니다.
또한 local, dev 환경 분리 작업을 하여 변경이 있을 속성만 분리 진행하였습니다.
prodution은 아직 사용할 단계가 아닌것같아 dev까지만 진행하였고 QA 서버에서 사용할정도로만 진행했습니다.
prodution 파일도 지금 부터 필요하면 추가 하겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
환경 분리 작업을 별도의 yml 생성이 아닌 프로필 분리로 통일해주세요.
https://docs.spring.io/spring-boot/reference/features/profiles.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요청 주신대로 찾아서 재작업하였습니다.
그 외 어떤 것들을 또 분기해야 할지 애매하여 데이터베이스 연결 부분만 진행하였습니다.
common에서 추가로 환경별 분리되어야 할 것이 있는지 확인 부탁드립니다!
src/main/resources/application.yml
Outdated
| "dev" : "common, dev" | ||
| active: local | ||
| --- | ||
| # application-local |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
불필요한 주석은 제거하셔도 좋을 것 같은데 주석을 별도로 쓰시는 이유가 있으실까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local 인지 dev 인지 확인하기 위해서 작성했는데 on-profile에 적혀 있어서 필요 없어보여 제거했습니다.
src/main/resources/application.yml
Outdated
|
|
||
| --- | ||
|
|
||
| # common |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 프로필은 뭔가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
공통 프로필입니다.
맨 위에 작성하니 나머지 dev, local에 대한 프로필이 너무 밑으로 밀러나서 지저분 해보이기도 하며,
local, dev 등을 맨위에서 관리하면 좋고, 공통으로 사용하게 되는것도 정리가 잘되는것 같은 생각으로 작성했습니다.
너무 불필요한 profile 이면 원래로 그룹별로 나누지 않고 맨위에있는 프로필로 다시 원복하겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
상단에 작성을 하게 되면 모든 프로필에 상관없이 동일한 프로퍼티가 적용됩니다.
공통으로 적용되어야 하는 프로퍼티는 굳이 프로필로 나눌 필요가 없기 때문에 상단 배치에 대한 코멘트를 드렸습니다.
| @@ -0,0 +1,59 @@ | |||
| package org.atdev.artrip.config; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 작성해달라고 했던 테스트 코드는 "비즈니스 요구사항"에 대한 테스트 코드였습니다.
프로필이 그에 해당하진 않는 것 같은데 이 부분 테스트 코드를 쓰신 이유가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네 제거했습니다.
| @@ -0,0 +1,32 @@ | |||
| package org.atdev.artrip.config; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
마찬가지
dnwls16071
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다.
마지막으로 코멘트 드렸던 부분까지 확인되셨다면 수정 후 머지부탁드립니다.
| @Configuration(proxyBeanMethods = false) | ||
| public class ProductionConfiguration { | ||
|
|
||
| @Bean | ||
| @Profile("local") | ||
| public DataSource locaDataSource() { | ||
| HikariDataSource dataSource = new HikariDataSource(); | ||
| dataSource.setJdbcUrl("jdbc:mysql://localhost:33069/artrip?serverTimezone=Asia/Seoul&useSSL=false&allowPublicKeyRetrieval=true"); | ||
| dataSource.setUsername("root"); | ||
| dataSource.setPassword("artrip1!"); | ||
| return dataSource; | ||
| } | ||
|
|
||
| @Bean | ||
| @Profile("dev") | ||
| public DataSource devDataSource( | ||
| @Value("${DB_HOST}") String dbHost, | ||
| @Value("${DB_PORT}") int dbPort, | ||
| @Value("${DB_NAME}") String dbName, | ||
| @Value("${DB_USERNAME}") String username, | ||
| @Value("${DB_PASSWORD}") String password | ||
| ) { | ||
|
|
||
| HikariDataSource dataSource = new HikariDataSource(); | ||
| String jdbcUrl = String.format("jdbc:mysql://%s:%d/%s?serverTimezone=Asia/Seoul&useSSL=false&allowPublicKeyRetrieval=true", | ||
| dbHost, dbPort, dbName); | ||
| dataSource.setJdbcUrl(jdbcUrl); | ||
| dataSource.setUsername(username); | ||
| dataSource.setPassword(password); | ||
| return dataSource; | ||
|
|
||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분에 대해 의아한 점이 있어서요.
ProductionConfiguration이라는 네이밍을 이해하면 "프로덕션용 데이터 소스를 위한 설정인가"라는 생각이 드는데 정작 내용을 보면 local, dev와 관련된 데이터 소스 설정이 있네요.
의도한 설계이신지 궁금합니다.
추가로 프로덕션용 DB는 아직 없다고 하셨기 때문에 RDS 인스턴스를 파실거라면 사실 이 부분이 지금 당장 필요는 없을 것 같아요.
그리고 오탈자 꼼꼼히 체크 부탁드립니다.
locaDataSource
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네이밍 변경하였습니다.
오탈자 수정했습니다.
#213