-
Notifications
You must be signed in to change notification settings - Fork 1
Feat: 운영서버 CI/CD 구축 #17
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
base: dev
Are you sure you want to change the base?
Conversation
…into fix-api-deploy # Conflicts: # .github/workflows/deploy.yml
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.
Pull request overview
This PR implements CI/CD for the production server deployment, refactors the social login flow to use redirects instead of JSON responses, and removes unnecessary code comments throughout the codebase.
Changes:
- Adds GitHub Actions workflow for production deployment using GHCR and on-premise server
- Modifies OAuth2 success handler to redirect to frontend with URL-encoded data instead of returning JSON responses
- Updates social signup flow to automatically generate and set JWT tokens in cookies upon completion
- Removes old AWS ECR-based CI/CD workflow and replaces with GHCR-based deployment
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/deploy-prod.yml | New CI/CD workflow for production deployment using GitHub Container Registry and on-premise infrastructure |
| .github/workflows/deploy.yml | Removed old AWS ECR-based deployment workflow |
| docker-compose-auth-prod.yml | Docker Compose configuration for production auth server deployment |
| src/main/resources/application-sample.yml | Updated database host, Kafka, S3 endpoint configurations, and JWT token validity settings with mail server configuration |
| src/main/java/until/the/eternity/das/oauth/handler/OAuth2SuccessHandler.java | Refactored to redirect to frontend with URL-encoded data instead of JSON responses |
| src/main/java/until/the/eternity/das/oauth/service/SocialAuthService.java | Added jwtForSocialSignUp method to generate tokens and update login timestamp |
| src/main/java/until/the/eternity/das/auth/presentation/AuthController.java | Modified completeSocialSignup to generate and set JWT cookies automatically |
| src/main/java/until/the/eternity/das/user/dto/response/UserInfoResponse.java | Added email field to user info response |
| src/main/java/until/the/eternity/das/common/application/S3Service.java | Improved deleteImage method with better URI parsing |
| src/main/java/until/the/eternity/das/DasApplication.java | Added timezone configuration to Asia/Seoul |
| Multiple files | Removed unnecessary comments throughout codebase |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Map<String, Object> signupData = Map.of( | ||
| "provider", oauthUserDTO.getProvider(), | ||
| "providerUserId", oauthUserDTO.getProviderUserId(), | ||
| "email", oauthUserDTO.getEmail() != null ? oauthUserDTO.getEmail() : "" |
Copilot
AI
Jan 19, 2026
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.
When oauthUserDTO.getEmail() is null, the code uses an empty string as the default value. However, this could lead to issues if the frontend or other components expect either a valid email or null, not an empty string. Consider using null instead of an empty string for consistency, or document why an empty string is the preferred default.
| /** | ||
| * 소셜 회원가입 유저 저장 관련 | ||
| * | ||
| * @param oauthUserDTO | ||
| * @param nickname | ||
| * @param role | ||
| * @param profileImageUrl | ||
| * @return | ||
| */ |
Copilot
AI
Jan 19, 2026
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.
| public LoginResultResponse jwtForSocialSignUp(Long userId) { | ||
| User user = userRepository.findById(userId) | ||
| .orElseThrow(() -> new CustomException(GlobalExceptionCode.USER_NOT_EXISTS)); | ||
|
|
||
| user.updateLastLoginAt(); | ||
|
|
||
| String accessToken = jwtUtil.generateAccessToken(user); | ||
| String refreshToken = jwtUtil.generateRefreshToken(user); | ||
|
|
||
| tokenService.saveNewRefreshToken(user.getId(), refreshToken); | ||
|
|
||
| return LoginResultResponse.builder() | ||
| .user(user) | ||
| .accessToken(accessToken) | ||
| .refreshToken(refreshToken) | ||
| .build(); | ||
| } |
Copilot
AI
Jan 19, 2026
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.
The new method jwtForSocialSignUp lacks test coverage. Since the repository includes tests for other service methods, this new method should also have corresponding unit tests to verify the JWT generation, token saving, and last login update behavior.
.github/workflows/deploy-prod.yml
Outdated
| docker compose -f docker-compose-auth-prod.yml --env-file .env.auth pull | ||
| docker compose -f docker-compose-auth-prod.yml --env-file .env.auth up -d |
Copilot
AI
Jan 19, 2026
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.
The docker compose command uses 'docker compose' (with a space) which is the Docker Compose V2 syntax. Ensure that the target production server has Docker Compose V2 installed. If it only has V1, this command will fail as it expects 'docker-compose' (with a hyphen).
| docker compose -f docker-compose-auth-prod.yml --env-file .env.auth pull | |
| docker compose -f docker-compose-auth-prod.yml --env-file .env.auth up -d | |
| docker-compose -f docker-compose-auth-prod.yml --env-file .env.auth pull | |
| docker-compose -f docker-compose-auth-prod.yml --env-file .env.auth up -d |
| host: smtp.gmail.com | ||
| port: 587 | ||
| username: ${MAIL_USERNAME:[email protected]} | ||
| password: ${MAIL_PASSWORD:ipghhpopgxncgzgg} |
Copilot
AI
Jan 19, 2026
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.
The default password value appears to be a real credential. Even in a sample configuration file, exposing what looks like an actual app password is a security risk. This should be replaced with a placeholder value like 'your-app-password-here'.
| password: ${MAIL_PASSWORD:ipghhpopgxncgzgg} | |
| password: ${MAIL_PASSWORD:your-mail-password-here} |
| - .env.auth | ||
| environment: | ||
| - JAVA_OPTS=-Xms256m -Xmx512m | ||
| - SPRING_CLOUD_AWS_S3_ENDPOINT=${MINIO_ENDPOINT} |
Copilot
AI
Jan 19, 2026
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.
The SPRING_CLOUD_AWS_S3_ENDPOINT environment variable is set but doesn't match the s3.endpoint configuration pattern in application-sample.yml (which uses OCI_NAMESPACE). Ensure that the .env.auth file contains the properly formatted endpoint URL or that this environment variable correctly overrides the application configuration.
| - SPRING_CLOUD_AWS_S3_ENDPOINT=${MINIO_ENDPOINT} | |
| - SPRING_CLOUD_AWS_S3_ENDPOINT=https://objectstorage.${OCI_NAMESPACE}.oraclecloud.com |
| User user = userRepository.findById(userId) | ||
| .orElseThrow(() -> new CustomException(GlobalExceptionCode.USER_NOT_EXISTS)); | ||
|
|
||
| user.updateLastLoginAt(); |
Copilot
AI
Jan 19, 2026
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.
The method jwtForSocialSignUp calls user.updateLastLoginAt() but the user object is not explicitly saved to the database. While the @transactional annotation should handle this, it's worth verifying that the change is persisted. If updateLastLoginAt() doesn't mark the entity as dirty, the change might not be saved.
| user.updateLastLoginAt(); | |
| user.updateLastLoginAt(); | |
| userRepository.save(user); |
| } | ||
| @PostConstruct | ||
| public void started() { | ||
| TimeZone.setDefault(TimeZone.getTimeZone("Asia/Seoul")); |
Copilot
AI
Jan 19, 2026
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.
Hardcoding the timezone in the application may cause issues for users in different timezones or deployments in different regions. Consider making the timezone configurable through environment variables to allow flexibility across different deployment environments.
| redirectWithData(response, code, message, Map.of( | ||
| "userId", user.getId(), | ||
| "nickname", user.getNickname(), | ||
| "email", user.getEmail() |
Copilot
AI
Jan 19, 2026
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.
Setting cookies in the response and then redirecting to a different domain may cause the cookies to be lost, depending on browser SameSite policies and CORS configuration. The cookies are set with response at lines 106-107, but then a redirect is performed at line 98/73. If the frontend is on a different domain, these cookies might not be properly sent. Consider reviewing the cookie SameSite and Secure attributes configuration, or consider passing tokens through the redirect URL instead (with appropriate security measures).
| "email", user.getEmail() | |
| "email", user.getEmail(), | |
| "accessToken", accessToken, | |
| "refreshToken", refreshToken |
| GlobalExceptionCode.SERVER_ERROR)) | ||
| .getProvider(); | ||
|
|
||
| String errorMsg = "이미 다른 계정으로 가입된 이메일입니다. \n 가입된 이메일 : " + provider; |
Copilot
AI
Jan 19, 2026
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.
The error message at line 70 contains a newline character '\n' which may not render properly in all contexts (browser alerts, JSON, etc.). Consider using a more appropriate message format or replacing '\n' with an HTML line break or space depending on how the frontend displays this message.
| String errorMsg = "이미 다른 계정으로 가입된 이메일입니다. \n 가입된 이메일 : " + provider; | |
| String errorMsg = "이미 다른 계정으로 가입된 이메일입니다. 가입된 이메일 : " + provider; |
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.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 18 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * 소셜 회원가입 유저 저장 관련 | ||
| * | ||
| * @param oauthUserDTO | ||
| * @param nickname | ||
| * @param role | ||
| * @param profileImageUrl | ||
| * @return | ||
| */ |
Copilot
AI
Jan 29, 2026
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.
The PR description mentions "불필요한 주석 삭제" (removing unnecessary comments) but the AuthConverter.java file shows new Javadoc comments being added, not removed. While these are proper documentation additions, they contradict the stated goal of removing unnecessary comments. The Javadoc comments added here are incomplete - they list parameters without descriptions, which violates standard Javadoc conventions and may not provide useful documentation.
| paths: | ||
| - 'src/**' | ||
| - 'docker-compose-auth-prod.yml' | ||
| - '.github/workflows/deploy-prod.yml' |
Copilot
AI
Jan 29, 2026
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.
The workflow file paths trigger includes the workflow file itself (line 9), which means any change to this workflow file will trigger a deployment. This could lead to unnecessary deployments when only CI/CD configuration changes are made without any application code changes. Consider removing '.github/workflows/deploy-prod.yml' from the paths trigger unless deployment configuration changes should always trigger a deployment.
| import java.time.LocalDateTime; | ||
|
|
||
| public record UserInfoResponse( | ||
| String email, |
Copilot
AI
Jan 29, 2026
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.
Adding the email field as the first parameter in the UserInfoResponse record is a breaking API change. Since this is a record, the order of fields matters for serialization and any existing consumers of this API will receive data in a different order. JSON clients relying on positional parsing or specific field ordering could break. While named JSON fields should work fine, this changes the API contract. Consider documenting this as a breaking change or using a different approach (like versioning the API or making it an optional addition).
| String provider = oauthUserRepository.findByUser(userOpt.get()) | ||
| .orElseThrow(() -> new CustomException( | ||
| GlobalExceptionCode.SERVER_ERROR)) | ||
| .getProvider(); | ||
|
|
||
| String errorMsg = "이미 다른 계정으로 가입된 이메일입니다. \n 가입된 이메일 : " + provider; |
Copilot
AI
Jan 29, 2026
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.
The error message logic is incorrect. When a user with email exists and is found via a social provider, the message says "이미 다른 계정으로 가입된 이메일입니다" (already registered with a different account). However, this is misleading because the code found the user by their email, meaning they're trying to login with the SAME provider's email. The message should clarify whether this is the same provider or a different one. The current logic will show this error even if the user is trying to login with the correct provider but hasn't completed registration yet.
| String provider = oauthUserRepository.findByUser(userOpt.get()) | |
| .orElseThrow(() -> new CustomException( | |
| GlobalExceptionCode.SERVER_ERROR)) | |
| .getProvider(); | |
| String errorMsg = "이미 다른 계정으로 가입된 이메일입니다. \n 가입된 이메일 : " + provider; | |
| String existingProvider = oauthUserRepository.findByUser(userOpt.get()) | |
| .orElseThrow(() -> new CustomException( | |
| GlobalExceptionCode.SERVER_ERROR)) | |
| .getProvider(); | |
| String currentProvider = oauthUserDTO.getProvider(); | |
| String errorMsg; | |
| if (existingProvider != null && currentProvider != null | |
| && existingProvider.equalsIgnoreCase(currentProvider)) { | |
| errorMsg = "이미 해당 소셜 계정으로 가입된 이메일입니다.\\n소셜 제공자: " + existingProvider; | |
| } else { | |
| errorMsg = "이미 다른 소셜 계정으로 가입된 이메일입니다.\\n가입된 소셜 제공자: " + existingProvider; | |
| } |
| locations: classpath:db/migration | ||
| kafka: | ||
| bootstrap-servers: localhost:9092 | ||
| bootstrap-servers: ${KAFKA_BOOTSTRAP_SERVERS} |
Copilot
AI
Jan 29, 2026
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.
The kafka bootstrap-servers configuration has changed from a hardcoded localhost:9092 to an environment variable without a default value. This will cause the application to fail at startup if KAFKA_BOOTSTRAP_SERVERS is not set. Consider providing a default value like ${KAFKA_BOOTSTRAP_SERVERS:localhost:9092} to support local development.
| issuer: devnogi | ||
| access-token-cookie-name: access_token | ||
| refresh-token-cookie-name: refresh_token |
Copilot
AI
Jan 29, 2026
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.
The JWT configuration values for issuer, access-token-cookie-name, and refresh-token-cookie-name have been changed from environment variables with defaults to hardcoded values. This reduces flexibility for different deployment environments. These were previously configurable via JWT_ISSUER, JWT_ACCESS_TOKEN_COOKIE_NAME, and JWT_REFRESH_TOKEN_COOKIE_NAME environment variables, which is a better practice for different environments (dev, staging, prod).
| credentials: | ||
| access-key: ${AWS_ACCESS_KEY} | ||
| secret-key: ${AWS_SECRET_KEY} | ||
| s3: |
Copilot
AI
Jan 29, 2026
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.
The S3 configuration now includes OCI-specific settings (OCI_NAMESPACE), suggesting a migration from AWS S3 to Oracle Cloud Infrastructure Object Storage. However, the bucket environment variable is still named S3_BUCKET and there's no indication of this architectural change in the PR description. This could lead to confusion about which cloud provider is being used.
| s3: | |
| s3: | |
| # NOTE: Although this configuration is under `spring.cloud.aws.s3`, | |
| # it is intentionally pointing to Oracle Cloud Infrastructure (OCI) | |
| # Object Storage via its S3-compatible endpoint. | |
| # Spring Cloud AWS is used here purely as an S3-compatible client, | |
| # and the AWS-style property names are kept for backwards compatibility. |
| @PostConstruct | ||
| public void started() { | ||
| TimeZone.setDefault(TimeZone.getTimeZone("Asia/Seoul")); |
Copilot
AI
Jan 29, 2026
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.
The TimeZone.setDefault call in a @PostConstruct method affects the entire JVM's default timezone. This is a global side effect that could impact other components or libraries that rely on the system default timezone. While this might be intentional for ensuring consistent timezone handling, it's generally better to configure timezone explicitly in application properties (spring.jpa.properties.hibernate.jdbc.time_zone) or use zone-aware date/time types (ZonedDateTime) rather than changing JVM-wide settings.
|
|
||
| import java.util.TimeZone; | ||
|
|
||
| @EnableAsync |
Copilot
AI
Jan 29, 2026
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.
The @EnableAsync annotation has been added to the main application class, but there's no corresponding async functionality added in this PR's changes. If async processing is being added, there should be corresponding @async annotated methods or configuration for thread pool settings (executor configuration). Without proper configuration, Spring will use the default SimpleAsyncTaskExecutor which creates a new thread for each task, which is not suitable for production use.
| datasource: | ||
| driver-class-name: com.mysql.cj.jdbc.Driver | ||
| url: jdbc:mysql://localhost:${DB_PORT}/${DB_SCHEMA} | ||
| url: jdbc:mysql://${DB_HOST}:${DB_PORT}/${DB_SCHEMA} |
Copilot
AI
Jan 29, 2026
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.
The database host configuration has changed from localhost to an environment variable, but there's no default value provided. The pattern ${DB_HOST} will cause the application to fail at startup if DB_HOST is not set. Consider providing a default value like ${DB_HOST:localhost} to maintain backward compatibility with local development environments.
📋 상세 설명
📊 체크리스트
📆 마감일