Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Hubobe 51 be 포토 포인트 #23
Hubobe 51 be 포토 포인트 #23
Changes from 68 commits
2299c0c
cff9d55
edfd104
88558f8
5301870
164fc94
e1ef165
d2d62b3
c360da4
45f7bc5
07331b8
981276e
a48e3ee
536b071
4afb6f3
07edda0
2899f1e
44809be
da8c713
31a66dc
2d68e4c
3d63f25
e0d42a4
4dab2f3
6ecb458
a1fad86
2336708
5068c1a
434f4e9
350044b
ab6bc87
f8407d3
76f4f9b
19ac2d9
57f327f
0a6ed7b
e9aded0
b2988f2
347dceb
628aadf
dbc91e1
f28e9e8
07fa376
cfa7fab
60fed31
8a619a3
1db6eeb
ff2788f
32f7747
0caa402
ecb3d31
4fd17b9
7df35f6
929e067
b6eb01d
2bcf7a2
e72c901
e0312cc
f96319f
fa417b2
ef9ff0e
cf41088
00a2a4c
24e5ecb
20a9e85
900568e
e64bdc7
d2cfa70
d0cc753
746623a
6498243
03fcfba
7431801
d63159d
3051a72
7916e8f
cc77cd5
0e68491
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
🛠️ Refactor suggestion
Consider using a non-root user for MySQL in the test environment
While using the root user is convenient for testing, it's a good practice to use a dedicated user with limited privileges, even in test environments. This helps to catch potential permission issues early and aligns better with security best practices.
Consider creating a dedicated test user for MySQL:
Then, update your application configuration to use these non-root credentials.
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.
🛠️ Refactor suggestion
Consider using a non-root user for MySQL in the test environment
While using the root user is convenient for testing, it's a good practice to use a dedicated user with limited privileges, even in test environments. This helps to catch potential permission issues early and aligns better with security best practices.
Consider creating a dedicated test user for MySQL:
Then, update your application configuration to use these non-root credentials.
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.
Use a separate secret for Elasticsearch password
Currently, the Elasticsearch password is set to the MySQL root password. This practice reduces security by reusing credentials across different services.
Create a separate secret for the Elasticsearch password and update the configuration:
Don't forget to add the new
ELASTICSEARCH_PASSWORD
secret to your GitHub repository secrets.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.
Consider using a separate secret for Elasticsearch password
The current configuration uses the MySQL root password for Elasticsearch. While this works, it's generally better to use separate secrets for different services to enhance security and make password rotation easier.
Consider creating a new secret specifically for Elasticsearch and updating the workflow:
Don't forget to add the new secret to your GitHub repository secrets.
📝 Committable suggestion
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.
🛠️ Refactor suggestion
Consider a more portable health check for MongoDB
The current health check uses
mongosh
, which might not be available in all environments. A more portable solution would be to use themongo
command-line tool, which is more commonly available.Update the MongoDB health check to use
mongo
:This change ensures better compatibility across different environments.
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.
Fix potential issue in Elasticsearch plugin installation
The current plugin installation step uses command substitution without quotes, which could lead to issues if the container name contains spaces or special characters.
Update the plugin installation command to use proper quoting:
This change ensures that the command works correctly regardless of the container name format.
🧰 Tools
🪛 actionlint
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.
🛠️ Refactor suggestion
Optimize Elasticsearch setup and verification process
The current Elasticsearch setup and verification process is thorough but can be streamlined for efficiency. Consider combining the wait, plugin installation, restart, and health check steps into a single step:
This combined step reduces the number of separate actions while maintaining the necessary checks and setup procedures.
🧰 Tools
🪛 actionlint
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.
🛠️ Refactor suggestion
Enhance MySQL health check
The MySQL health check is well-implemented, but there are a few minor improvements that can be made.
Consider the following enhancements:
Fix the quoting issue in the mysqladmin command:
Use English for error messages to ensure clarity for international teams:
Add a connection timeout to prevent potential hangs:
These changes will improve the robustness and clarity of the MySQL health check.
🧰 Tools
🪛 actionlint
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.
🛠️ Refactor suggestion
Optimize MySQL setup process
The current MySQL setup process executes multiple commands individually, which can be optimized for efficiency.
Consider combining all MySQL operations into a single connection:
This change will make the setup more efficient while maintaining comprehensive error reporting. Also, consider creating a separate user for the application instead of using root for better security.
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.
🛠️ Refactor suggestion
Optimize MySQL setup and use English for messages
The current MySQL setup process is thorough but can be optimized. Additionally, using English for messages ensures better understanding across international teams.
Consider the following improvements:
These changes improve efficiency, readability, and internationalization of the MySQL setup process.
🧰 Tools
🪛 actionlint
🪛 yamllint
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.
🛠️ Refactor suggestion
Enhance security in Elasticsearch debug output
While the debug information is valuable for troubleshooting, it might expose sensitive data in CI logs.
Consider the following improvements:
These changes help protect sensitive information while still providing valuable debug output when needed.
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.
🛠️ Refactor suggestion
Consider removing commented-out remote server actions.
The workflow file contains commented-out steps for starting and stopping a test environment on a remote server. If these steps are no longer needed due to the shift towards a self-contained CI environment, it's best to remove them entirely rather than leaving them as comments.
If you're certain that the remote server actions are no longer required, remove the commented-out code to improve the clarity and maintainability of the workflow file. If there's a possibility that you might need these steps in the future, consider moving them to a separate document or creating a git branch with these changes for future reference.
Also applies to: 146-156
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.
🛠️ Refactor suggestion
Remove commented-out remote server actions
The presence of commented-out code for remote server actions indicates a shift in testing strategy from remote to local testing. Keeping unused code, even as comments, can lead to confusion and make the workflow file harder to maintain.
Consider removing these commented-out sections entirely to improve the clarity and maintainability of the workflow file. If there's a possibility that you might need these steps in the future, consider moving them to a separate document or creating a git branch with these changes for future reference.
Also applies to: lines 229-239
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.
Remove redundant Lombok test dependencies
The Lombok library is already included as a compile-time dependency (compileOnly and annotationProcessor). Adding it again as a test implementation is unnecessary and redundant. Moreover, it's added twice, which could lead to confusion.
Please remove these lines:
The existing Lombok dependencies should be sufficient for both compile-time and test-time usage.
📝 Committable suggestion
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.
Adjust Spring Security Test dependency
The Spring Security Test library is typically used for testing purposes and should be a test dependency rather than an implementation dependency. Additionally, it's good practice to align the version with the Spring Boot version used in the project.
Please make the following changes:
Apply this diff:
This change ensures that:
📝 Committable suggestion
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.
🛠️ Refactor suggestion
Consider the implications of adding @builder annotation
The addition of the
@Builder
annotation provides a flexible way to createCity
objects, which can be beneficial for objects with multiple fields. However, consider the following points:The class already has a static factory method
createCity()
. Ensure that the team agrees on when to use the builder pattern versus the factory method to maintain consistency across the codebase.The
@Builder
annotation generates an all-args constructor, which might conflict with the existing@AllArgsConstructor
. Consider using@Builder(toBuilder = true)
to allow creating new instances from existing ones.If you decide to keep both the builder and the factory method, consider updating the
createCity()
method to use the builder internally for consistency.Consider updating the
createCity()
method to use the builder:This change would make the factory method consistent with the new builder pattern.
Check warning on line 29 in src/main/java/com/hubo/gillajabi/course/application/presenation/CourseBookMarkController.java
Codecov / codecov/patch
src/main/java/com/hubo/gillajabi/course/application/presenation/CourseBookMarkController.java#L27-L29
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.
Improve response handling in
toggleCourseBookmark
There are a few issues with the current implementation:
ResponseEntity
without a generic type, which reduces type safety.Consider the following improvements:
ResponseEntity<String>
.This approach provides more accurate feedback to the client about the action performed.
Check warning on line 38 in src/main/java/com/hubo/gillajabi/course/application/presenation/CourseBookMarkController.java
Codecov / codecov/patch
src/main/java/com/hubo/gillajabi/course/application/presenation/CourseBookMarkController.java#L37-L38
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.
🛠️ Refactor suggestion
Consider renaming
toggleCourseBookmark
to reflect its functionality.The method
toggleCourseBookmark
no longer toggles the bookmark status; it only adds a bookmark and throws an exception if it already exists. Renaming the method toaddCourseBookmark
orcreateCourseBookmark
would make its purpose clearer.Check warning on line 33 in src/main/java/com/hubo/gillajabi/course/domain/service/CourseBookMarkService.java
Codecov / codecov/patch
src/main/java/com/hubo/gillajabi/course/domain/service/CourseBookMarkService.java#L33
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.
🛠️ Refactor suggestion
Use a custom exception instead of
RuntimeException
.Throwing a general
RuntimeException
can make error handling less precise. Consider creating a custom exception likeBookmarkAlreadyExistsException
to provide more specific context.Example:
Check warning on line 38 in src/main/java/com/hubo/gillajabi/course/domain/service/CourseBookMarkService.java
Codecov / codecov/patch
src/main/java/com/hubo/gillajabi/course/domain/service/CourseBookMarkService.java#L36-L38
Check warning on line 42 in src/main/java/com/hubo/gillajabi/course/domain/service/CourseBookMarkService.java
Codecov / codecov/patch
src/main/java/com/hubo/gillajabi/course/domain/service/CourseBookMarkService.java#L42
Check warning on line 45 in src/main/java/com/hubo/gillajabi/course/domain/service/CourseBookMarkService.java
Codecov / codecov/patch
src/main/java/com/hubo/gillajabi/course/domain/service/CourseBookMarkService.java#L44-L45
Check warning on line 48 in src/main/java/com/hubo/gillajabi/course/domain/service/CourseBookMarkService.java
Codecov / codecov/patch
src/main/java/com/hubo/gillajabi/course/domain/service/CourseBookMarkService.java#L48
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.
🛠️ Refactor suggestion
Use a more appropriate exception for unauthorized access.
Throwing
IllegalArgumentException
may not accurately represent an authorization issue. Consider usingAccessDeniedException
or creating a custom exception to indicate lack of permission.Example:
📝 Committable suggestion
Check warning on line 51 in src/main/java/com/hubo/gillajabi/course/domain/service/CourseBookMarkService.java
Codecov / codecov/patch
src/main/java/com/hubo/gillajabi/course/domain/service/CourseBookMarkService.java#L51
Check warning on line 8 in src/main/java/com/hubo/gillajabi/course/infrastructure/exception/CourseBookMarkException.java
Codecov / codecov/patch
src/main/java/com/hubo/gillajabi/course/infrastructure/exception/CourseBookMarkException.java#L8
Check warning on line 10 in src/main/java/com/hubo/gillajabi/course/infrastructure/exception/CourseBookMarkException.java
Codecov / codecov/patch
src/main/java/com/hubo/gillajabi/course/infrastructure/exception/CourseBookMarkException.java#L10
Check warning on line 12 in src/main/java/com/hubo/gillajabi/course/infrastructure/exception/CourseBookMarkException.java
Codecov / codecov/patch
src/main/java/com/hubo/gillajabi/course/infrastructure/exception/CourseBookMarkException.java#L12
Check warning on line 19 in src/main/java/com/hubo/gillajabi/course/infrastructure/exception/CourseBookMarkException.java
Codecov / codecov/patch
src/main/java/com/hubo/gillajabi/course/infrastructure/exception/CourseBookMarkException.java#L15-L19