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

Integrating composite triggers with the DeploymentTrigger YAML representation #12413

Merged
merged 5 commits into from
Mar 26, 2024

Conversation

chrisguidry
Copy link
Collaborator

@chrisguidry chrisguidry commented Mar 25, 2024

It wasn't apparent to me the first time that just backporting the Trigger hierarchy and TriggerTypes wasn't enough to get them to be available in deployments. This adds them to the DeploymentTrigger schema so that folks can define them in their prefect.yaml files and get full support of the nested triggers.

@chrisguidry chrisguidry requested a review from a team as a code owner March 25, 2024 20:14
@chrisguidry chrisguidry added the enhancement An improvement of an existing feature label Mar 25, 2024
Copy link

netlify bot commented Mar 25, 2024

Deploy Preview for prefect-docs-preview ready!

Name Link
🔨 Latest commit c291d42
🔍 Latest deploy log https://app.netlify.com/sites/prefect-docs-preview/deploys/6603189775003c0008a29025
😎 Deploy Preview https://deploy-preview-12413--prefect-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@serinamarie serinamarie left a comment

Choose a reason for hiding this comment

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

Do you think it'd be a good idea to add some integration tests like in our
test_deployments.py

  • TestDeploymentBasicInterface
  • TestDeploymentApply

and like tests for malformed triggers that are missing required fields?

@chrisguidry
Copy link
Collaborator Author

That's a great call, @serinamarie, I'll try to get some of those in today

@chrisguidry
Copy link
Collaborator Author

It turns out that we do have a set of integration tests over in tests/cli/test_deploy.py to make sure that we can parse these into automations when creating deployments, so that's good.

I'm still not satisfied with how this works, so I'm going to do some more work on it to reflect the real triggers class hierarchy

As I was starting to write more tests for DeploymentTrigger, I realized that
I was effectively reproducing tests for validation that Pydantic would do for
us if we just matched the hierarchy correctly.  Unfortunately there's no
way to inject a few fields into a base class for another class hierarchy, so I'm
reproducing the full hierarchy and its validate rules here.
@chrisguidry chrisguidry force-pushed the composite-triggers-in-deployments branch from f808c80 to 159d02d Compare March 26, 2024 14:28
@@ -207,6 +207,93 @@ def test_compound_deployment_trigger_as_automation():
)


def test_deeply_nested_compound_deployment_trigger_as_automation():
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@chrisguidry chrisguidry merged commit e173697 into main Mar 26, 2024
46 checks passed
@chrisguidry chrisguidry deleted the composite-triggers-in-deployments branch March 26, 2024 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants