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

Refactor advanced task args argument validators #3571

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

Conversation

bomoko
Copy link
Contributor

@bomoko bomoko commented Oct 17, 2023

This provides a (hopefully) better and simpler abstraction for validating advanced task definition arguments.

The function getAdvancedTaskArgumentValidator(sqlClientPool, environment, taskArguments) is a scaffolding function that returns an instance of the class AdvancedTaskArgumentValidator, into which it injects everything that's needed to validate incoming argument values for the given task arguments (given via the taskArguments parameter).

This change serves two purposes

  1. It gets rid of the somewhat confusing advancedTaskDefinitionTypeFactory and tries to provide better encapsulation of the validation functionality.
  2. Introduces some basic testing for the argument validators
  3. By keeping DB calls in the getAdvancedTaskArgumentValidator function, we remove any dependencies that were inside the validators themselves (the environment validators were doing DB calls), opting to instead inject them.

General Checklist

  • Affected Issues have been mentioned in the Closing issues section
  • Documentation has been written/updated
  • PR title is ready for inclusion in changelog

@bomoko bomoko requested review from rocketeerbkw and removed request for rocketeerbkw November 2, 2023 21:25
@bomoko bomoko marked this pull request as draft November 3, 2023 01:17
@bomoko
Copy link
Contributor Author

bomoko commented Nov 3, 2023

Converting to draft - there's a logic issue when this isn't attached directly to an environment.

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

Successfully merging this pull request may close these issues.

1 participant