-
Notifications
You must be signed in to change notification settings - Fork 948
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
Updated time duration to 30 seconds #5594
Conversation
Hello @yoyounik, can you tell us more about your PR ? |
hello @Skraye
please let me know if you need more details or have any questions! Thanks for reviewing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discuss here #4879 the issue has been misunderstood
Please make the change and restore the test as it was
hello @Skraye Added a new Java class: io.kestra.plugin.core.debug Note: I've temporarily ignored writing test cases to expedite the PR review process. I'll add comprehensive test cases in a follow-up commit once the core functionality is approved. Please review these changes and let me know if you have any questions or require further clarification. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closed to perfection! Small changes and we are good, you can add a test :)
hey @anna-geller @Skraye Additionally, any feedback on the SequentialTest.java test case would be greatly appreciated. |
@Example( | ||
code = """ | ||
id: wait | ||
type: wait.task |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a valid type, the example won't work
@@ -0,0 +1,55 @@ | |||
package io.kestra.plugin.core.debug; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The package is not the good one, here is the one asked in the original issue :
io.kestra.plugin.core.flow.Sleep
with io.kestra.plugin.core.flow
being the package and Sleep
being the class name
and this will form the task type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yoyounik Please read this, better take your times to make correct changes once than doing it 10 times
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Skraye I apologize for the repeated mistakes. I will take my time to ensure that the changes are correct before submitting them again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Skraye
Thanks for the feedback. You're right, "Sleep" might be a more descriptive name for the class.
I can rename the Wait.java class to Sleep.java and update the code accordingly. This would keep the file in the io.kestra.plugin.core.flow package.
Does that sound like a good approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exactly what's need to be done 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay then i will rename it and update you @Skraye
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Skraye
changes i made:
Wait.java changed to Sleep.java,
wait-task-flow.yml changed to sleep-task-flow.yml and changes inside it to Sleep.
sleep.yml's type has been changed.
going to update these changes now, please go through this once, and thanks for the patience shown :).
Please update me if any changes required.
id: sleep-task-flow | ||
namespace: io.kestra.tests | ||
|
||
tasks: | ||
- id: sleep | ||
type: io.kestra.core.tasks.scripts.Bash | ||
script: sleep 30 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove this ? Never use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done @Skraye
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete the file, not just remove the content please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hello @Skraye
Updated the changes according to your review, Thanks alot for the review , if you have time to review it once more , please check and tell if there is need of any more modifications.
type: io.kestra.plugin.core.debug.Wait duration: "PT5S"
type: io.kestra.plugin.core.debug.Wait duration: "PT5S" Also updated the wait.java in the flow package
@Example( | ||
code = """ | ||
id: wait | ||
type: io.kestra.plugin.core.debug.Wait |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong type
|
||
tasks: | ||
- id: wait | ||
type: io.kestra.plugin.core.debug.Wait |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong type
wait-task-flow.yml changed to sleep-task-flow.yml and changes inside it to Sleep. sleep.yml's type has been changed.
e111a7e
to
c8b5e88
Compare
We have a full list that are good for learning to work on the project : Pick a new one in the list, take your times to understand the issue correctly, and feel free to open another PR! |
Thank you! @Skraye I'll take a look at the list and make sure to fully understand the issues before opening another PR :) |
What changes are being made and why?
How the changes have been QAed?
Setup Instructions