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

Tasks #313

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Tasks #313

wants to merge 7 commits into from

Conversation

dpanshug
Copy link
Collaborator

@dpanshug dpanshug commented Jan 19, 2023

Copy link
Collaborator

@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.

I need the build working here

Copy link
Collaborator

@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.

The empty message is not ok

Screenshot 2023-03-06 at 17 15 49

Tooltip message seems incomplete

Screenshot 2023-03-06 at 17 16 46

Counter name is required

Screenshot 2023-03-06 at 17 17 27

@karesti
Copy link
Collaborator

karesti commented Mar 6, 2023

package.json lock should not be changed

@karesti
Copy link
Collaborator

karesti commented Mar 6, 2023

which script have you used to test the upload?
would be good for some testing @dpanshug

@domiborges
Copy link
Member

I have the docs locally. I'll push it tomorrow. The only thing I'm missing is the tooltip. I'm not sure what to put there.

@karesti
Copy link
Collaborator

karesti commented Mar 7, 2023

@dpanshug needs rebase

@domiborges
Copy link
Member

I need some hint what to put in the tooltip. https://infinispan.org/docs/stable/titles/server/server.html#remote-execution I see the tasks can be scripts or custom Java classes. Is it necessary to stop the server? etc.

@dpanshug dpanshug force-pushed the ISPN-14305-tasks branch 2 times, most recently from 2bf5393 to f98bdfd Compare March 13, 2023 05:55
@dpanshug
Copy link
Collaborator Author

dpanshug commented Mar 13, 2023

@dvagnero need to merge your commit with mine for some changes

@karesti
Copy link
Collaborator

karesti commented Mar 13, 2023

@dpanshug let me know when this is ready again plz

@dpanshug
Copy link
Collaborator Author

which script have you used to test the upload? would be good for some testing @dpanshug

Writing a E2E test for this.

@dpanshug
Copy link
Collaborator Author

I need some hint what to put in the tooltip. https://infinispan.org/docs/stable/titles/server/server.html#remote-execution I see the tasks can be scripts or custom Java classes. Is it necessary to stop the server? etc.

@tristantarrant can you help here pls.

docs suggestions for tasks ISPN-14305
@dpanshug dpanshug requested a review from andyuk1986 March 14, 2023 09:46
@dpanshug
Copy link
Collaborator Author

@andyuk1986 Added a cypress test, pls review :)

@andyuk1986
Copy link
Collaborator

Cypress test looks good but scenario for task edit/save is not there.

@dpanshug
Copy link
Collaborator Author

@dvagnero Found this in documentation

You can implement tasks as custom Java classes or define scripts in languages such as JavaScript.`

Maybe this info is useful for the tooltip.

@domiborges
Copy link
Member

@dvagnero Found this in documentation

You can implement tasks as custom Java classes or define scripts in languages such as JavaScript.`

Maybe this info is useful for the tooltip.

Okay so it can be Java or Javascript. What about requirements? Having a script engine? Anything else?

@dpanshug
Copy link
Collaborator Author

Okay so it can be Java or Javascript. What about requirements? Having a script engine? Anything else?

For the JS task, you will need to install a JS script engine on the server, it won't work with JDK 17.

@domiborges
Copy link
Member

Thank you for suggestions @dpanshug
Is it necessary to stop the server before adding a task?

@tristantarrant
Copy link
Member

Thank you for suggestions @dpanshug Is it necessary to stop the server before adding a task?

If it's a Java task, yes. Script tasks can be added at runtime

@andyuk1986
Copy link
Collaborator

@dvagnero just to add my two cents - as @tristantarrant said, there are 2 types of tasks: remote server tasks and Script Tasks. Console shows both Remote and Script Tasks. But at the moment (with this PR) it is possible to create/edit/execute only Script Tasks, and - no, for these tasks no Server restart is needed.
For Remote tasks the process is different -> jar file should be copied to server's data folder and server need to be restarted but I don't know if these use case will be covered with Console in the future.

Copy link
Collaborator

@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.

  • When the task creation gives an error, the error should be displayed in the top of the popup and not in the alert. Otherwise we lose what we wrote in the editor.
    First validate that the task was created, and if it's ok, close the popup and display an alert of ok.
  • The task list should display the task that has been created

image

If the user can't see the task, we should not be displaying any alert but only listing the names of the tasks and disable the actions the user can't do.

image

image

@domiborges
Copy link
Member

Thanks @andyuk1986 for the explanation. I was under the impression that we're aiming for both tasks and scripts.
So @dpanshug and @karesti do you plan to add support for tasks written in Java? And if yes, would users use the same process to upload either a script or task?
Tasks and scripts have some similarities but also a different workflow and prerequisites (script engine, stopping server etc). What do you think about having a radio button to choose the Task type inside the Create a task dialogue? Or some other way to differentiate between these two?

@tristantarrant
Copy link
Member

There is no way to upload a Java task: it needs to be deployed before startup

@dpanshug
Copy link
Collaborator Author

dpanshug commented Mar 24, 2023

The task list should display the task that has been created

@karesti I've tried fixing it, list gets updated when there already exist some tasks. But if you try to create the task for the first time, the API seems to not fetch the tasks list without refreshing separately. Can you please help me out here. :)

@domiborges
Copy link
Member

Hi @dpanshug I changed the tooltip. Let me know if you need anything or if you make any changes that require docs :)

@karesti
Copy link
Collaborator

karesti commented Mar 27, 2023

@dpanshug check how the entries add works plz

@dpanshug dpanshug requested a review from karesti March 29, 2023 07:47
@dpanshug
Copy link
Collaborator Author

@karesti Pls check if this can be merged :)

@karesti
Copy link
Collaborator

karesti commented Apr 14, 2023

@dpanshug no, this feature can't be merged as it is. if the javascript engine is not available, we should disable the task creation. it is confusing and useless. for that we need an endpoint in the scripts that indicates that an js engine is available.
I'm unable to test and understand he feature (as a user from the console).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tasks - CRUD management
5 participants