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

r/trigger: re-introduce query_json for deeper validation #487

Merged
merged 2 commits into from
Jun 14, 2024

Conversation

jharley
Copy link
Collaborator

@jharley jharley commented Jun 14, 2024

In v0.2.0 we dropped query_json from r/trigger to smooth out few a gnarly bugs associated with the Plugin SDK. With that change the ability to more deeply validate that a Trigger's query met the requirements was sadly lost.

Since then, r/trigger has been re-written in the new Plugin Framework and with it we got the ability to determine if the configuration was building via Query ID or Query JSON. This change takes advantage of that ability and re-introduces deeper validation for Trigger query's so long as you use query_json in place of query_id

@jharley jharley added the feature This issue wants to add new functionality. label Jun 14, 2024
@jharley jharley requested a review from a team as a code owner June 14, 2024 14:13
Comment on lines +505 to +508
// exit early if we don't have QueryJSON
if data.QueryJson.IsNull() || data.QueryJson.IsUnknown() {
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does returning early here mean it successfully validated or that it failed?

Copy link
Collaborator Author

@jharley jharley Jun 14, 2024

Choose a reason for hiding this comment

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

Neither: we can't validate anything that isn't there

Edit: which, I guess is "success" in a sense. Validators generate warnings or errors if there is an issue but otherwise allow things to continue

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, maybe this is a mental model fail on my part -- I was thinking about the line above

if resp.Diagnostics.HasError() {
		return
	}

which to me read a bit like "if there's an existing error the config of the trigger resource fails validation". But maybe it means something closer to "if there's an existing error [or we don't have queryJSON]", don't run further validation"? If that's right, that makes sense 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a different error case: it means we were unable to get the configuration so we fail out right away.

These functions don't have returned results (no boolean or error), but instead add to a *resource.ValidateConfigResponse -- often with the intention of adding more than one error or warning -- but there are also cases where you know it's not safe to continue.

@jharley jharley merged commit 6a2eaa4 into main Jun 14, 2024
3 checks passed
@jharley jharley deleted the jharley.return-of-trigger-queryjson branch June 14, 2024 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue wants to add new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants