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

Added REST methods for task and trigger validation that accept JSON body #3785

Merged
merged 4 commits into from
Sep 25, 2024

Conversation

rybandrei2014
Copy link
Contributor

What changes are being made and why?

Kestra doesn't provide rest endpoints for validation of task and triggers in JSON format. Endpoint for task/trigger validation /validate/task accepts only YAML body.
Following PR adds two new endpoints:

  • /validate/task - endpoint for task validation, accepts JSON body
  • /validate/trigger - endpoint for trigger validation, accepts JSON body

@loicmathieu
Copy link
Member

@rybandrei2014
Copy link
Contributor Author

Hi @loicmathieu,

no worries, thanks for your reaction.

I added unit tests for new endpoints.

Thank you

Copy link
Member

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

Can you move all your tests data inline in the Java test class so they are more easily found and didn't add too many files.

You can use text blocks to deal with double quotes, for ex:

String task = """
    {
      "id": "task_one",
      "type": "io.kestra.plugin.core.log.Log"
    }
"""

@@ -533,6 +538,53 @@ public List<ValidateConstraintViolation> validateFlows(
.collect(Collectors.toList());
}

@ExecuteOn(TaskExecutors.IO)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@ExecuteOn(TaskExecutors.IO)
// This endpoint is not used by the Kestra UI nor our CLI but is provided for the API users for convenience
@ExecuteOn(TaskExecutors.IO)

Adding a comment will prevent someone removing the method as not used ;)

return validateConstraintViolationBuilder.build();
}

@ExecuteOn(TaskExecutors.IO)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@ExecuteOn(TaskExecutors.IO)
// This endpoint is not used by the Kestra UI nor our CLI but is provided for the API users for convenience
@ExecuteOn(TaskExecutors.IO)

@loicmathieu
Copy link
Member

@rybandrei2014 ca you address my comments so we can proceed with the PR?

Copy link
Member

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

@rybandrei2014
Copy link
Contributor Author

@rybandrei2014 ca you address my comments so we can proceed with the PR?

Hello @loicmathieu, I am currently busy with my other project. Hopefully, I will be able to provide requested changes next week.

Sorry for keeping you waiting and late reply

@loicmathieu
Copy link
Member

@rybandrei2014 I made the necessary change and will now merge it.
Thanks again for your contribution.

@loicmathieu loicmathieu merged commit 2e96612 into kestra-io:develop Sep 25, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants