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

Separating application.properties into different files based on profile #563

Draft
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

ishish07
Copy link

@ishish07 ishish07 commented Aug 12, 2024

What's changed?

I added 1 recipe to split application.properties files into multiple application-profile.properties files if additional profiles are specified and separated by either #--- or !---

This recipe also deletes the additional profile configurations in the application.properties file itself as it would be replaced by a new file.

What's your motivation?

During my internship this summer, I had to manually create new application-environment.properties files based on the profiles and/or environments that were specified in application.properties. This was during our migration to Java 21. We were doing this for organizational purposes.

Anything under #--- or !--- in application.properties belongs to a different document. If there are global properties under #--- or !---, those configurations will not be applied to other profiles and/or environments and could cause bugs. By running this recipe and separating application-env.properties files, it becomes obvious if there are missing global properties in application.properties.

Anything in particular you'd like reviewers to focus on?

I would like the reviewers to focus on whether the file path to the newly created application-env.properties files is accurate. These new files belong in the same folder that application.properties exists in.

I would also like the reviewers to see if there is any way to add a helpful comment when appending properties into an existing application-profile.properties file. I append new content on line 92 in the appendToExistingPropertiesFile method. I was not sure how to add a comment as we are dealing with Properties.Content instances. It would be nice to add something like

"# Below are the properties from application.properties!"

The third thing I'd like reviewers to focus on is whether there is an easy way to have the code do nothing if no application.properties file is found. I was trying to use Precondition.check in getScanner but I wasn't able to get it to work. There are a few if statements littered around the code just to deal with the case that the application.properties file doesn't exist, so it would make the code a lot more clean if it could just do nothing when it detects that in the scanner.

Thank you!

Any additional context

Most likely, respositories will not maintain a profile specific section in application.properties along with an existing application-profile.properties file. However, I felt it would be safe to keep this edge case in mind. I wrote the recipe to safely append content to its corresponding pre-existing application-profile.properties file - otherwise the recipe would run and harmfully delete configurations.

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

@timtebeek timtebeek self-requested a review August 12, 2024 08:07
@timtebeek timtebeek added the recipe Recipe requested label Aug 12, 2024
@timtebeek
Copy link
Contributor

Thanks a lot @ishish07 ! I like how you've sketched the context of having had to do this by hand, and now wish to automate this for others going forward. I'll need to take a closer look & go over the focus areas you've identified, but expect we can get this in somewhere this week.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

Great start here @ishish07 ! Thanks for your patience in the review. I've pushed up a few polishing commits already, but think we have some work to do still where multi module projects are involved. Are you ok to make that change still?

Comment on lines +140 to +141
map.put("application-" + ((Properties.Entry) newContent.get(0)).getValue().getText() + ".properties",
newContent.subList(1, newContent.size()));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use the full target file name as key here, since there could be a multi module project that could clash.

parent
  - childA
    - src/main/resource/application.properties
  - childB
    - src/main/resource/application.properties

Copy link
Author

Choose a reason for hiding this comment

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

I believe the code already adjusts for files within nested folders like your example (line 206 of the testing file). I also do not think that a project can have multiple application.properties files.

The method above - getNewApplicationPropertyFileInfo is simply for getting the content of each application-env.properties file from application.properties. Application-env.properties files that don't exist will, by default, be placed in the same folder as the application.properties file.

Copy link
Contributor

Choose a reason for hiding this comment

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

hi! I've just pushed a test to highlight what I meant; 4f23297
The test at line 206 only had a single folder structure, not two separate.

Given the scale at which recipes can run I think we should account for such a scenario.

@timtebeek timtebeek marked this pull request as draft September 11, 2024 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
recipe Recipe requested
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants