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

Change painless scripted field #21026

Merged
merged 3 commits into from
Jul 20, 2018
Merged

Change painless scripted field #21026

merged 3 commits into from
Jul 20, 2018

Conversation

timroes
Copy link
Contributor

@timroes timroes commented Jul 20, 2018

Due to elastic/elasticsearch#32207 our functional tests started failing on master.

This PR changes the scripted field, that was failing to do the appropriate check if an actual value is present in the document.

@timroes timroes added bug Fixes for quality problems that affect the customer experience :Management v7.0.0 labels Jul 20, 2018
bmcconaghy
bmcconaghy approved these changes Jul 20, 2018
View reviewed changes
Copy link
Contributor

@bmcconaghy bmcconaghy left a comment

Choose a reason for hiding this comment

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

LGTM

@elasticmachine
Copy link
Contributor

💔 Build Failed

nreese
nreese approved these changes Jul 20, 2018
View reviewed changes
Copy link
Contributor

@nreese nreese left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lgtm
code review

Throwing an error for missing fields is a huge change that is going to cause a lot of upgrade pain for customers. Any reason why ES made this change?

@nreese
Copy link
Contributor

nreese commented Jul 20, 2018

Looks like the new script does not compile. Needs to be changed to

        if (doc['machine.ram'].size() == 0) {
          return -1;
        } else {
          return doc['machine.ram'].value / (1024 * 1024 * 1024);
        }

I removed the / and there was a extra tick after "1024)"

nreese
nreese reviewed Jul 20, 2018
View reviewed changes
test/functional/apps/management/_scripted_fields.js Outdated
const script = `if (doc[\'machine.ram\'].size() == 0) {
return -1;
} else {
return doc[\'machine.ram\'].value / (1024 * 1024 * 1024)';
Copy link
Contributor

Choose a reason for hiding this comment

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

There is an extra ' at the end of this line that is invalid

@nreese nreese mentioned this pull request Jul 20, 2018
Merged
cjcenizal
cjcenizal approved these changes Jul 20, 2018
View reviewed changes
Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

LGTM thanks for hopping on this so quickly everyone.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@cjcenizal
Copy link
Contributor

Retest

@cjcenizal
Copy link
Contributor

The CI failed on X-Pack:

15:11:17    │ proc  [ftr]          └- ✖ fail: "security app Management Security navigation Can navigate to edit user section"
15:11:17    │ proc  [ftr]          │        tryForTime timeout: NoSuchElement: An element could not be located on the page using the given search parameters.

image

@nreese
Copy link
Contributor

nreese commented Jul 20, 2018

Retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@cjcenizal
Copy link
Contributor

We made it!

@nreese nreese merged commit 0dbaf65 into elastic:master Jul 20, 2018
nreese pushed a commit to nreese/kibana that referenced this pull request Jul 20, 2018
@timroes @nreese
Change painless scripted field (elastic#21026)
f99f6af 
* Change painless scripted field

* Remove wrong char

* Remove invalid escape chars
@nreese nreese mentioned this pull request Jul 20, 2018
Closed
@nreese nreese added the v6.4.0 label Jul 20, 2018
@nreese nreese mentioned this pull request Jul 20, 2018
Merged
nreese added a commit that referenced this pull request Jul 20, 2018
@nreese
Change painless scripted field (#21026) (#21046)
54d4c53 
* Change painless scripted field

* Remove wrong char

* Remove invalid escape chars
@timroes timroes deleted the fix-es-change branch July 23, 2018 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nreese nreese nreese approved these changes

@bmcconaghy bmcconaghy bmcconaghy approved these changes

@cjcenizal cjcenizal cjcenizal approved these changes

Assignees
No one assigned
Labels
bug Fixes for quality problems that affect the customer experience v6.4.0 v7.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@timroes @elasticmachine @nreese @cjcenizal @bmcconaghy