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

chore: make pre-deployment changes #79

Merged
merged 5 commits into from
Dec 6, 2023

Conversation

harshithl1777
Copy link
Owner

Summary of Changes

  • Added Dockerfile and .dockerignore
  • Fixed CORS configuration in RelayApplication.java and added null checks in AuthenticationFilter.java

Copy link
Collaborator

@muhammadIbrahim-cpu muhammadIbrahim-cpu left a comment

Choose a reason for hiding this comment

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

lgtm

@harshithl1777 harshithl1777 merged commit 68a54bb into main Dec 6, 2023
1 check passed
@vivic04 vivic04 self-requested a review December 6, 2023 21:30
@vivic04
Copy link
Collaborator

vivic04 commented Dec 6, 2023

RelayApplication.java:
The addition of the CORS configuration is good for allowing specific origins. However, consider making the allowed origins configurable through properties or environment variables to allow easier updates without code changes.
It might be beneficial to create constants for the allowed origins to avoid hardcoding them directly in the code.
The CORS configuration could be moved to a separate class or package for better organization, especially if there are more configuration options in the future.

@vivic04
Copy link
Collaborator

vivic04 commented Dec 6, 2023

AuthenticationFilter.java:
Instead of printing the social token to the console (System.out.println(socialToken)), consider using proper logging mechanisms for better visibility and maintainability.
It's a good practice to log the exception stack trace in case of an error to aid in debugging.
The check for a valid social token could be more robust. Ensure that the code handles all possible scenarios, and consider using a utility method or library for token validation.
The code for splitting the authorization header and checking for null could be refactored for clarity and to avoid potential issues.

@vivic04
Copy link
Collaborator

vivic04 commented Dec 6, 2023

ci_test.yml:
The change in the GitHub Actions workflow to use JDK 21 is appropriate for keeping the development environment up to date.
Ensure that all necessary dependencies and plugins are compatible with JDK 21.
Consider adding comments explaining the reasons for choosing JDK 21 in the workflow file.

@vivic04
Copy link
Collaborator

vivic04 commented Dec 6, 2023

After updating the JDK version, ensure that the application still builds and runs successfully. Check for any deprecated APIs or breaking changes.

@vivic04
Copy link
Collaborator

vivic04 commented Dec 6, 2023

Overall seems good, a few minor changes and we are good to go!

This was referenced Dec 6, 2023
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.

None yet

3 participants