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

Standardize version property names #2460

Open
wants to merge 7 commits into
base: integration
Choose a base branch
from

Conversation

foster33
Copy link
Collaborator

@foster33 foster33 commented Jul 5, 2024

This PR is part of the effort to standardize version property names across all repos to make it easier to update the version of an artifact across the board.

Also updates documentation in the BUILDME regarding property names.

Related to: DW Issue 2436

@alerman
Copy link
Collaborator

alerman commented Jul 8, 2024

I feel like moving the properties this direction is a mistake. It makes us lose the implied grouping of the microservices. Id be fine with changing it to version.somethingBesidesMicroservervices.* if we don't like using that name across all the repositories, but I don't like losing the information that this was part of one of our services. How does a new developer differentiate one of our microservices from something we don't build?

We already get confusion between version.accumulo and version.microservices.accumulo-utils, removing microservices will only make this worse.

@jwomeara
Copy link
Collaborator

jwomeara commented Jul 8, 2024

I feel like moving the properties this direction is a mistake. It makes us lose the implied grouping of the microservices. Id be fine with changing it to version.somethingBesidesMicroservervices.* if we don't like using that name across all the repositories, but I don't like losing the information that this was part of one of our services. How does a new developer differentiate one of our microservices from something we don't build?

We already get confusion between version.accumulo and version.microservices.accumulo-utils, removing microservices will only make this worse.

Luke and I talked about this before he started working on it. At the time, I had bought into the idea of removing the 'microservice' part of the property with the idea being that the property names were probably unique enough without that part, and that keeping things short would be nice. I can see the benefit of keeping a common prefix in the property name - having the properties grouped together is certainly nice. Maybe we should consider something other than 'microservice'.

What about 'version.datawave.____' instead? Keep in mind that not all of the properties prefixed with 'microservice' were actually microservices (e.g. type-utils, metadata-utils, accumulo-utils, etc.). You could argue that the various api jars are not really 'microservice' jars either as they're depended on by both the microservices and datawave.

The overall goal of this effort was to standardize the names across all projects so that we could update the version of a thing all at once. This will facilitate being able to run a full snapshot build of all of the services in a github action, which i think would be good to do.

What are both of your thoughts on removing 'microservice' and instead using 'datawave' since these are all datawave artifacts? I think that would give everyone what they want.

@foster33
Copy link
Collaborator Author

foster33 commented Jul 8, 2024

What are both of your thoughts on removing 'microservice' and instead using 'datawave' since these are all datawave artifacts? I think that would give everyone what they want.

I think going with 'version.datawave.____' makes sense to me.

@alerman
Copy link
Collaborator

alerman commented Jul 9, 2024

I agree that version.datawave._____ works.

ivakegg
ivakegg previously approved these changes Jul 9, 2024
ivakegg
ivakegg previously approved these changes Jul 10, 2024
jwomeara
jwomeara previously approved these changes Jul 16, 2024
Copy link
Collaborator

@jwomeara jwomeara left a comment

Choose a reason for hiding this comment

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

I like it a lot. Can you link the related PRs for our other repos so we can merge all of these at once? (assuming you've finished those already)

@foster33
Copy link
Collaborator Author

I like it a lot. Can you link the related PRs for our other repos so we can merge all of these at once? (assuming you've finished those already)

I linked all of the related PRs to the initial issue, there should be a list there (#2436)

@foster33 foster33 dismissed stale reviews from jwomeara, keith-ratcliffe, and ivakegg via 4dad491 July 18, 2024 14:27
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

5 participants