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.

Sign up for GitHub

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

Jump to bottom

feat(config): unify properties files #2766

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

feat(config): unify properties files #2766

wants to merge 4 commits into from

Conversation

GMishx
Copy link
Member

@GMishx GMishx commented Dec 2, 2024 •
edited
Loading

Please provide a summary of your changes here.

  1. The PR unifies all the different sw360.properties resources into a single file under datahandler.
  2. The load of these properties are unified org.eclipse.sw360.datahandler.common.SW360Constants where applicable.
  3. Additional classes are created as needed under the org.eclipse.sw360.datahandler.common package extending the SW360Constants class.
  4. New actuator for exposing the constants from these classes. The actuator deliberately exposes only the configs needed the the UI to work. The backend only configs are not exposed. This can be changed if needed.

Closes #2765

Suggest Reviewer

@smrutis1 @heliocastro

How To Test?

Check the /api/config endpoint.

Checklist

Must:

  • All related issues are referenced in commit messages and in PR

This PR needs changes from #2709 and should be merged after it.

GMishx added 3 commits December 2, 2024 15:51
@GMishx
fix(openapi)!: add health endpoint to openapi
114c1a2 
Rearrange the application.yml in test to look more like source.

BREAKING CHANGE: Move the health endpoint from `/resource/health` to
`/resource/api/health` for OpenAPI documentations to be easy to use.

Signed-off-by: Gaurav Mishra <[email protected]>
@GMishx
chore(properties): unify property locations
c8ecccc 
Unify all `sw360.properties` files to a single file in datahandler
library.

Also, move all loaders for the property file to SW360Constants class and
datahandler.common package.

Signed-off-by: Gaurav Mishra <[email protected]>
@GMishx
feat(api): expose properties as /config actuator
95397c9 
Add a new actuator at `/config` to expose the contents of properties
files.

Signed-off-by: Gaurav Mishra <[email protected]>
@GMishx GMishx added needs code review needs general test This is general testing, meaning that there is no org specific issue to check for New-UI Level for the API and UI level changes for the new-ui labels Dec 2, 2024
@GMishx GMishx force-pushed the feat/config/unify-config branch from 50e6d13 to 63cc839 Compare December 2, 2024 14:02
heliocastro
heliocastro previously approved these changes Dec 2, 2024
View reviewed changes
Copy link
Contributor

@heliocastro heliocastro left a comment

Choose a reason for hiding this comment

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

Sounds good. I think need a second pair of eyes to review it.

smrutis1
smrutis1 reviewed Dec 3, 2024
View reviewed changes
libraries/datahandler/src/main/java/org/eclipse/sw360/datahandler/common/SW360Constants.java
public static final Boolean REST_IS_JWKS_VALIDATION_ENABLED;
public static final String REST_SERVER_PATH_URL;

// UI Portlet Constants
Copy link
Contributor

Choose a reason for hiding this comment

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

I was just wondering if UI constants are required anymore or it can be sanitized?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will recheck once which constants from this section are used by backend and which are not. The ones not used can be moved to the frontend project.

My initial thought was to have minimal changes the existing sw360.properties files. But yes, the best approach would be to separate the backend and frontend configs.

@heliocastro
Copy link
Contributor

@GMishx Question: Do we need a property files at all ?
Can we simply move everything to the database and create default entries and rest api to update.
This would simplify a lot.

@GMishx GMishx force-pushed the feat/config/unify-config branch from 63cc839 to e94180c Compare December 4, 2024 06:43
@GMishx GMishx force-pushed the feat/config/unify-config branch from e94180c to fb171c9 Compare December 4, 2024 06:43
@GMishx
Copy link
Member Author

GMishx commented Dec 4, 2024

@GMishx Question: Do we need a property files at all ? Can we simply move everything to the database and create default entries and rest api to update. This would simplify a lot.

That would be the ultimate goal. If anyone is interested in doing can work on it. Till we have a working release with the new front-end and back-end, it would be a low priority imo.

But the solution from this PR would be good for the front-end to at least have idea how the back-end is configured.

@hoangnt2
Copy link
Contributor

hoangnt2 commented Dec 4, 2024

@heliocastro @GMishx, I am currently investigating and implementing a POC for storing some configurations in CouchDB.

My idea is to store configurations in the sw360config database in CouchDB (similar to the FOSSology configuration). This approach will allow users to modify them through the GUI. However, I don't intend to move all configurations from the properties file into the database, as some are related to migration scripts and the connections between back-end services.

I will create a PR for this in a few weeks.

@GMishx
Copy link
Member Author

GMishx commented Dec 4, 2024

That sounds awesome @hoangnt2 , we can hold this PR then and use it just to unify the remaining configuration files.

@smrutis1 smrutis1 added the has merge conflicts The PR has merge conflicts label Dec 11, 2024
@GMishx
Copy link
Member Author

GMishx commented Dec 16, 2024

Putting this PR on hold in favor of #2815

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smrutis1 smrutis1 smrutis1 left review comments

@heliocastro heliocastro heliocastro left review comments

@KoukiHama KoukiHama Awaiting requested review from KoukiHama KoukiHama will be requested when the pull request is marked ready for review KoukiHama is a code owner

@arunazhakesan arunazhakesan Awaiting requested review from arunazhakesan arunazhakesan will be requested when the pull request is marked ready for review arunazhakesan is a code owner

@ag4ums ag4ums Awaiting requested review from ag4ums ag4ums will be requested when the pull request is marked ready for review ag4ums is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
do not merge - нет! has merge conflicts The PR has merge conflicts needs code review needs general test This is general testing, meaning that there is no org specific issue to check for New-UI Level for the API and UI level changes for the new-ui
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Endpoint to expose all configs needed for the UI to be in-sync with backend (N)
4 participants
@GMishx @heliocastro @hoangnt2 @smrutis1