Skip to content

Conversation

alpar-t
Copy link
Contributor

@alpar-t alpar-t commented May 29, 2019

With this change we only have to add one line to add a new version.
The intent is to make it less error prone and easier to write a script
to automate the process.

There are tools to generate source code or byte-code that would allow us to
automate version bumps based on a plain list of versions,
but that would create friction with IDEs.
There are also tools that semantically understand java source and are able to reformat it,
but unfortunately there's no existing tooling to edit a java source file.
With this change it should be straight forward to automate it by manipulating the source
as a plain text file.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

Looks good, I left some comments.

Copy link
Member

@jasontedor jasontedor May 29, 2019

Choose a reason for hiding this comment

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

Let's also:

  • only add for fields named V_\d+_\d+_\d+
  • assert the only field not matching this name is CURRENT
  • assert that the id is only added to the map once (currently it's not because V_8_0_0 and CURRENT)

Copy link
Member

Choose a reason for hiding this comment

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

Also, let's assert that the id matches the value derived from the name, so that we can reduce the possibility of error even further!

Copy link
Member

Choose a reason for hiding this comment

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

Let's use a primitive map, ImmutableOpenIntMap.

@jasontedor
Copy link
Member

@atorok I pushed a commit with my suggestions.

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

LGTM, but since I pushed commits I think that @rjernst should review too. 😇

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

This is a long needed change, thanks! LGTM

@mark-vieira
Copy link
Contributor

There are tools to generate source code or byte-code that would allow us to
automate version bumps based on a plain list of versions,
but that would create friction with IDEs.

If we want the best tooling available, using an annotation processor would probably be the best bet. Modern IDEs have support for those. I'm generally not a fan of code generation, but if storing version info in a structured file and generating the constants makes version bumping less of a pain then I'm all for it.

@jasontedor
Copy link
Member

@elasticmachine run elasticsearch-ci/2

@jasontedor
Copy link
Member

I'm generally not a fan of code generation, but if storing version info in a structured file and generating the constants makes version bumping less of a pain then I'm all for it.

I’m not a fan either. And anyway @rjernst and me have discussed how to do this across the stack. Code generation isn’t effective here because now we need generators for a bunch of different projects when most version bumps are capably done by sed. Only Elasticsearch is special but it can be done with text manipulation too, this PR makes it even easier.

@alpar-t
Copy link
Contributor Author

alpar-t commented May 30, 2019

@elasticmachine run elasticsearch-ci/2

1 similar comment
@alpar-t
Copy link
Contributor Author

alpar-t commented May 30, 2019

@elasticmachine run elasticsearch-ci/2

@alpar-t
Copy link
Contributor Author

alpar-t commented Jun 13, 2019

I'm not sure what happened with the history here, I am rebasing on master to fix it without any additional changes

@alpar-t alpar-t force-pushed the simplify-version-bump-changes branch from 1f6ad22 to 24007a8 Compare June 13, 2019 06:54
alpar-t and others added 6 commits June 13, 2019 09:55
With thic change we only have to add one line to add a new version.
The intent is to make it less error prone and easier to write a script
to automate teh process.
@alpar-t alpar-t merged commit ace1d40 into elastic:master Jun 13, 2019
@alpar-t alpar-t deleted the simplify-version-bump-changes branch June 13, 2019 16:33
alpar-t added a commit to alpar-t/elasticsearch that referenced this pull request Jun 13, 2019
With this change we only have to add one line to add a new version.
The intent is to make it less error prone and easier to write a script
to automate the process.
alpar-t added a commit to alpar-t/elasticsearch that referenced this pull request Jun 13, 2019
With this change we only have to add one line to add a new version.
The intent is to make it less error prone and easier to write a script
to automate the process.
alpar-t added a commit that referenced this pull request Jun 14, 2019
With this change we only have to add one line to add a new version.
The intent is to make it less error prone and easier to write a script
to automate the process.
alpar-t added a commit that referenced this pull request Jun 14, 2019
With this change we only have to add one line to add a new version.
The intent is to make it less error prone and easier to write a script
to automate the process.
henningandersen pushed a commit to henningandersen/elasticsearch that referenced this pull request Jun 17, 2019
…lastic#43215)

With this change we only have to add one line to add a new version.
The intent is to make it less error prone and easier to write a script
to automate the process.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants