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

ISPN-11042 TaskResource should allow filtering admin tasks #7648

Merged
merged 1 commit into from Dec 7, 2019
Merged

ISPN-11042 TaskResource should allow filtering admin tasks #7648

merged 1 commit into from Dec 7, 2019

Conversation

gustavocoding
Copy link

karesti
karesti reviewed Dec 5, 2019
View reviewed changes
Copy link
Contributor

@karesti karesti left a comment

Choose a reason for hiding this comment

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

LGTM if there is no other place where the validation of the name should be added to avoid users creating '@@' tasks

server/rest/src/main/java/org/infinispan/rest/resources/TasksResource.java Outdated
@@ -64,10 +64,15 @@ public Invocations getInvocations() {
}

private CompletionStage<RestResponse> createScriptTask(RestRequest request) {
NettyRestResponse.Builder builder = new NettyRestResponse.Builder();
Copy link
Contributor

Choose a reason for hiding this comment

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

appart from REST, there is no other place to create a task where the name validation should be added?

Copy link
Author

Choose a reason for hiding this comment

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

I don't know for sure, it appears the only way is to get hold of a ScriptManager and register a task

Copy link
Author

Choose a reason for hiding this comment

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

This will only be possible for embedded though. Hot Rod does not allow the creation of tasks

Copy link
Author

Choose a reason for hiding this comment

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

I think I will remove this validation stuff. This should be part of another JIRA, what we are doing here is to filter out internal tasks...

Copy link
Author

Choose a reason for hiding this comment

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

ok, done

karesti
karesti approved these changes Dec 5, 2019
View reviewed changes
Copy link
Contributor

@karesti karesti left a comment

Choose a reason for hiding this comment

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

LGTM if is good for Jenkins

@karesti
Copy link
Contributor

karesti commented Dec 5, 2019

related test failing in Cli
Expected output did not contain expected string after timeout expected:<[@@cache@names]> but was:<[[rhos-infinispan-ci-3-23441@org.infinispan.server.cli.CliIT//containers/default]> cd tasks

@gustavocoding
Copy link
Author

Ok, looks like CLI wants to see the internal tasks, but the console doesn't...What should it be then? 😄

@gustavocoding
Copy link
Author

gustavocoding commented Dec 5, 2019 •
edited

I think all the internal tasks perform actions that are available via REST, so they are kind of redundant, will fix that test

@gustavocoding gustavocoding changed the title ISPN-11042 TaskResource should not display internal tasks ISPN-11042 TaskResource should allow filtering admin tasks Dec 6, 2019
@gustavocoding
Copy link
Author

Ok @karesti, I changed my mind 😄
Admin tasks will be showed by default, unless you add ?type=user to the request

@gustavocoding
Copy link
Author

Failures in thread leaks on the SoftFileIndex can't be related to this, so CI is fine

@tristantarrant tristantarrant merged commit 889c1dd into infinispan:master Dec 7, 2019
@tristantarrant
Copy link
Member

Merged, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karesti karesti karesti approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@gustavocoding @karesti @tristantarrant