Skip to content

[Transform] Implement the ability to preview the existing transform#76697

Merged
przemekwitek merged 5 commits into
elastic:masterfrom
przemekwitek:transform_preview_by_id
Aug 24, 2021
Merged

[Transform] Implement the ability to preview the existing transform#76697
przemekwitek merged 5 commits into
elastic:masterfrom
przemekwitek:transform_preview_by_id

Conversation

@przemekwitek

@przemekwitek przemekwitek commented Aug 19, 2021 •
edited by droberts195
Loading

Copy link
Copy Markdown

This PR adds the ability to perform transform preview using already existing transform config.
The user does not have to provide the full config in the preview request anymore.
Instead, they can just provide transform id using id param.

Closes #76427

@przemekwitek przemekwitek force-pushed the transform_preview_by_id branch 3 times, most recently from 509733c to fbd85d4 Compare August 19, 2021 12:16
@sethmlarson sethmlarson added the Team:Clients Meta label for clients team label Aug 19, 2021
@przemekwitek przemekwitek force-pushed the transform_preview_by_id branch 3 times, most recently from ddf74f4 to 7e1afdc Compare August 23, 2021 09:32
@przemekwitek przemekwitek removed the WIP label Aug 23, 2021
@przemekwitek przemekwitek marked this pull request as ready for review August 23, 2021 09:36
@elasticmachine

elasticmachine commented Aug 23, 2021

Copy link
Copy Markdown
Collaborator

Pinging @elastic/clients-team (Team:Clients)

@przemekwitek przemekwitek added :ml/Transform Transform Team:ML Meta label for the ML team labels Aug 23, 2021
@elasticmachine

elasticmachine commented Aug 23, 2021

Copy link
Copy Markdown
Collaborator

Pinging @elastic/ml-core (Team:ML)

benwtrent
benwtrent reviewed Aug 23, 2021
View reviewed changes
Comment thread ...core/src/main/java/org/elasticsearch/xpack/core/transform/action/PreviewTransformAction.java Outdated
Comment thread x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/transform/preview_transforms.yml Outdated
Comment thread .../src/main/java/org/elasticsearch/xpack/transform/rest/action/RestPreviewTransformAction.java Outdated
Comment thread ...core/src/main/java/org/elasticsearch/xpack/core/transform/action/PreviewTransformAction.java Outdated
hendrikmuhs
hendrikmuhs reviewed Aug 24, 2021
View reviewed changes

@hendrikmuhs hendrikmuhs left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Note: I reviewed before the last commit, so probably some of my comments are outdated.

Comment thread .../src/main/java/org/elasticsearch/xpack/transform/rest/action/RestPreviewTransformAction.java Outdated
Comment thread x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/transform/preview_transforms.yml Outdated
@przemekwitek przemekwitek force-pushed the transform_preview_by_id branch from 7c8dcdf to 99e308b Compare August 24, 2021 07:03
@przemekwitek przemekwitek force-pushed the transform_preview_by_id branch from 8154c36 to 17150df Compare August 24, 2021 08:06
hendrikmuhs
hendrikmuhs reviewed Aug 24, 2021
View reviewed changes

@hendrikmuhs hendrikmuhs left a comment •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM, I have 1 more request: can you add a test in transform/qa/multi-cluster-tests-with-security/ to assert that bob is not able to preview by id the transform that joe created (or any other user combo)?

(I mean of course w.r.t. to data access rights: bob can preview joe's transform, but should not get access to data which only joe is allowed to retrieve)

@przemekwitek

przemekwitek commented Aug 24, 2021

Copy link
Copy Markdown
Author

LGTM, I have 1 more request: can you add a test in transform/qa/multi-cluster-tests-with-security/ to assert that bob is not able to preview by id the transform that joe created (or any other user combo)?

Done, I've added an assertion to an existing test.

hendrikmuhs
hendrikmuhs approved these changes Aug 24, 2021
View reviewed changes

@hendrikmuhs hendrikmuhs left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

@przemekwitek przemekwitek merged commit 676d4de into elastic:master Aug 24, 2021
@przemekwitek przemekwitek deleted the transform_preview_by_id branch August 24, 2021 12:41
@przemekwitek przemekwitek mentioned this pull request Aug 24, 2021
Merged
przemekwitek pushed a commit that referenced this pull request Aug 24, 2021
[Transform] Implement the ability to preview the existing transform (#…
d94d571 
…76697) (#76886)
@droberts195 droberts195 mentioned this pull request Sep 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benwtrent benwtrent benwtrent approved these changes
+1 more reviewer
@hendrikmuhs hendrikmuhs hendrikmuhs approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

>enhancement :ml/Transform Transform Team:Clients Meta label for clients team Team:ML Meta label for the ML team v7.16.0 v8.0.0-alpha2

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Transform] Preview should work against an existing id

8 participants

@przemekwitek @elasticmachine @benwtrent @hendrikmuhs @jakelandis @sethmlarson @danhermann @elasticsearchmachine