Skip to content

[7.17] Adjust date conversion for execution context in watcher compare condition#88467

Merged
stu-elastic merged 3 commits intoelastic:7.17from
sakurai-youhei:issue88408-add-test-and-fix
Jul 13, 2022
Merged

[7.17] Adjust date conversion for execution context in watcher compare condition#88467
stu-elastic merged 3 commits intoelastic:7.17from
sakurai-youhei:issue88408-add-test-and-fix

Conversation

@sakurai-youhei
Copy link
Copy Markdown
Member

@sakurai-youhei sakurai-youhei commented Jul 12, 2022

Issue: #88408

This PR backports the commit in the below PR to 7.17:

In addition to that, 58d57be will be cherry-picked once confirming CI build fails on the added test.

Question: Does the following changelog look ok?

pr: <to be replaced with PR #>
summary: Adjust date conversion for execution context in watcher compare condition
area: Infra/Scripting
type: bug
issues:
 - 88408

@sakurai-youhei
Add test of date math with ctx.execution_time
59c70f1 
Add one test to confirm the below watcher compare condition is always
met.

```
  "condition": {
    "compare": {
      "ctx.execution_time": {
        "gte": "<{now-5m}>"
      }
    }
  }
```
@elasticsearchmachine elasticsearchmachine added v7.17.6 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Jul 12, 2022
@DJRickyB DJRickyB added the :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache label Jul 12, 2022
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jul 12, 2022
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Jul 12, 2022

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

@sakurai-youhei
Add conditional branch for JodaCompatibleZonedDateTime
df128f1 
v1 can be an instance of JodaCompatibleZonedDateTime in versions prior
to elastic#78417, which has to be converted to ZonedDateTime for appropriate
comparison.

Closes elastic#88408
@sakurai-youhei
Add changelog for elastic#88467
df96d8d
@sakurai-youhei
Copy link
Copy Markdown
Member Author

sakurai-youhei commented Jul 12, 2022 •
edited
Loading

The added test failed expectedly at the beginning as below.

https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request+part-2/17006/console
21:01:21 org.elasticsearch.xpack.watcher.condition.CompareConditionTests > testExecuteDateMathExecutionContext FAILED
21:01:21 java.lang.AssertionError:
21:01:21 Expected: is <true>
21:01:21 but: was <false>
21:01:21 at __randomizedtesting.SeedInfo.seed([845D8CB26022E5A4:AD7E8F50A1E70CF2]:0)
21:01:21 at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:18)
21:01:21 at org.junit.Assert.assertThat(Assert.java:956)
21:01:21 at org.junit.Assert.assertThat(Assert.java:923)
21:01:21 at org.elasticsearch.xpack.watcher.condition.CompareConditionTests.testExecuteDateMathExecutionContext(CompareConditionTests.java:199)

And the fact that the subsequent commit df128f1 let the test pass proves the proper bug fix.

@elasticmachine elasticmachine added the Team:Data Management (obsolete) DO NOT USE. This team no longer exists. label Jul 13, 2022
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Jul 13, 2022

Pinging @elastic/es-data-management (Team:Data Management)

@rjernst rjernst removed the backport label Jul 13, 2022
@stu-elastic stu-elastic self-requested a review July 13, 2022 16:50
stu-elastic
stu-elastic approved these changes Jul 13, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@stu-elastic stu-elastic 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 0f64f22 into elastic:7.17 Jul 13, 2022
stu-elastic pushed a commit that referenced this pull request Jul 13, 2022
@sakurai-youhei
Watcher: Add test of date math with ctx.execution_time (#88466)
ece52f8 
Add one test to confirm the below watcher compare condition is always
met.

```
  "condition": {
    "compare": {
      "ctx.execution_time": {
        "gte": "<{now-5m}>"
      }
    }
  }
```

Refs: #88408 #88467
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@stu-elastic stu-elastic stu-elastic approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

>bug :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache :Distributed/Watcher external-contributor Pull request authored by a developer outside the Elasticsearch team Team:Core/Infra Meta label for core/infra team Team:Data Management (obsolete) DO NOT USE. This team no longer exists. v7.17.6

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@sakurai-youhei @elasticmachine @stu-elastic @rjernst @DJRickyB @elasticsearchmachine