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

Script: Rename TIMESTAMP constant NOW in Metadata #88870

Merged
merged 1 commit into from
Jul 27, 2022

Conversation

stu-elastic
Copy link
Contributor

The value is _now and there was a previous metadata
value _timestamp (see test removal in #88733) so the
name is confusing.

Also renames the method getTimestamp() to getNow()
to reflect the change.

The value is `_now` and there was a previous metadata
value `_timestamp` (see test removal in elastic#88733) so the
name is confusing.

Also renames the method `getTimestamp()` to `getNow()`
to reflect the change.
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.

The rename seems good, but I think we need to provide backcompat in scripts.

@@ -35,7 +35,7 @@ class org.elasticsearch.script.Metadata {
String getVersionType()
void setVersionType(String)

ZonedDateTime getTimestamp()
Copy link
Member

Choose a reason for hiding this comment

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

We need to provide backcompat for existing scripts. It could be done through an augmentation so it is isolated to just scripts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that the case for Metadata? It's in 8.4 but not documented or supported.

Copy link
Member

Choose a reason for hiding this comment

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

Well, 8.4 has just been branched. So I guess this could change, but it will need to go back to that branch (it should be fine, since it was literally just branched today).

@stu-elastic stu-elastic added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >refactoring and removed WIP labels Jul 27, 2022
@stu-elastic stu-elastic marked this pull request as ready for review July 27, 2022 20:17
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Jul 27, 2022
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

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.

LGTM

@stu-elastic stu-elastic merged commit fb01f4e into elastic:main Jul 27, 2022
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this pull request Jul 27, 2022
The value is `_now` and there was a previous metadata
value `_timestamp` (see test removal in elastic#88733) so the
name is confusing.

Also renames the method `getTimestamp()` to `getNow()`
to reflect the change.
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.4

elasticsearchmachine pushed a commit that referenced this pull request Jul 27, 2022
The value is `_now` and there was a previous metadata
value `_timestamp` (see test removal in #88733) so the
name is confusing.

Also renames the method `getTimestamp()` to `getNow()`
to reflect the change.
@mark-vieira mark-vieira added v8.4.0 and removed v8.4.1 labels Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >refactoring Team:Core/Infra Meta label for core/infra team v8.4.0 v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants