Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

feature: RunTrigger gRPC endpoint for Waypoint Server #2840

Merged
merged 48 commits into from
Jan 12, 2022

Conversation

briancain
Copy link
Member

@briancain briancain commented Dec 15, 2021

This pull request introduces the service level function RunTrigger which will queue a job based on an existing stored trigger URL configuration using the ID requested. It also optionally allows for any configuration variable overrides on the request.

I kept the targeted runner to Any until that work is finished. It will be easier once the targeted config is available based on the project and application.

The next step after this is to work on the HTTP handler. It should register a trigger URL id as a valid route, and if a request to that endpoint is called it will take a run trigger request with a registered trigger URL id and call this grpc func (similar to how the HTTP exec works) to queue the job request and optionally stream back the job outputs to the requester.

I tested this endpoint using the waypoint-grpc helper tool in contrib: https://github.com/hashicorp/waypoint/tree/main/contrib/waypoint-grpc

@briancain briancain added the pr/no-changelog No automatic changelog entry required for this pull request label Dec 15, 2021
@briancain briancain requested a review from a team December 15, 2021 21:22
@github-actions github-actions bot added the core label Dec 15, 2021
log.Debug("variable overrides have been requested for trigger job")
for i, v := range req.VariableOverrides {
switch vType := v.Source.(type) {
case *pb.Variable_Cli:
Copy link
Member Author

Choose a reason for hiding this comment

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

@krantzinator suggested maybe we could introduce a RunTrigger source type here too. Although the CLI type does exactly what we want here, I think.

Copy link
Contributor

@catsby catsby left a comment

Choose a reason for hiding this comment

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

Looks good so far! I had a few nit-picks and some questions about the copy. Probably no blockers but wanted to ask first

internal/cli/trigger_inspect.go Outdated Show resolved Hide resolved
internal/cli/trigger_inspect.go Outdated Show resolved Hide resolved
internal/server/singleprocess/service_trigger.go Outdated Show resolved Hide resolved
internal/server/singleprocess/service_trigger.go Outdated Show resolved Hide resolved
internal/server/singleprocess/state/job.go Outdated Show resolved Hide resolved
@briancain briancain requested a review from catsby January 3, 2022 22:31
@briancain briancain removed the pr/no-changelog No automatic changelog entry required for this pull request label Jan 4, 2022
@krantzinator
Copy link
Contributor

Quick comment -- can we make the error message for a missing op type a bit more informative?

! Empty operation type. Operation type must be set with the `-op` flag as one of the following values:
  build, push, deploy, destroy-workspace, destroy-deployment, release, up, init

@krantzinator
Copy link
Contributor

krantzinator commented Jan 5, 2022

Hm I'm getting some odd screen wrapping behavior when I have multiple triggers:
Screen Shot 2022-01-05 at 2 42 18 PM

Looks like this is just in general, as it doesn't do well when the screen isn't wide enough.
Maybe we should present minimal information in the list and only show more in json format or something? And prioritize getting this in the UI as well.

Oh, idea! Maybe list returns just that -- a list of IDs and/or Names. And then leave the detailed version to inspect. This would also be a good use case for filters...

Screen Shot 2022-01-05 at 2 43 37 PM

@krantzinator
Copy link
Contributor

krantzinator commented Jan 5, 2022

These cases should all have operation removed from the sentence (so just build instead of build operation). The column heading is Operation so it's redundant.
https://github.com/hashicorp/waypoint/blob/f564326568b7eff4496aec3ec9b61151d2fd677d/internal/cli/trigger_inspect.go Line 87-111

	case *pb.Trigger_Build:
		opStr = "build operation"

@krantzinator
Copy link
Contributor

Bug: trigger inspect has an id flag but also requires an argument that it uses for <trigger-id>. It should use one or the other. I hit a panic, as you would expect, when I used the flag but did not provide an argument.

@briancain
Copy link
Member Author

briancain commented Jan 5, 2022

@krantzinator - thanks for the feedback!

  • I've updated waypoint trigger list to only show a few options by default, and included a -full flag to show everything I was showing before.
  • I fixed the panic in the CLI if you specify an id by flag but not argument for the waypoint trigger inspect command.
  • Removed the operation text from inspect and list
  • Included which flag to set if no operation is set

Copy link
Contributor

@krantzinator krantzinator left a comment

Choose a reason for hiding this comment

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

Could you add a note on trigger-specific variables? I want to test it but it's not covered in the docs :)

website/content/docs/triggers.mdx Outdated Show resolved Hide resolved
website/content/docs/triggers.mdx Outdated Show resolved Hide resolved
website/content/docs/triggers.mdx Outdated Show resolved Hide resolved
website/content/docs/triggers.mdx Outdated Show resolved Hide resolved
website/content/docs/triggers.mdx Outdated Show resolved Hide resolved
website/content/docs/triggers.mdx Outdated Show resolved Hide resolved
website/content/docs/triggers.mdx Outdated Show resolved Hide resolved
website/content/docs/triggers.mdx Outdated Show resolved Hide resolved
website/content/docs/triggers.mdx Outdated Show resolved Hide resolved
Co-authored-by: Rae Krantz <8461333+krantzinator@users.noreply.github.com>
Copy link
Contributor

@krantzinator krantzinator left a comment

Choose a reason for hiding this comment

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

I have more testing I want to do when HTTP hits, but this looks good!

@briancain
Copy link
Member Author

Thank you all who took the time to review and test! Super helpful, appreciate you all ❤️ 🙏🏻

@briancain briancain merged commit 36a7f5f into main Jan 12, 2022
@briancain briancain deleted the feat/core/run-triggers branch January 12, 2022 22:47
@paladin-devops
Copy link
Contributor

When this is available for HTTP I'd love to try to configure Docker webhooks to use this (to kick off my docker-pull builds)!

@briancain
Copy link
Member Author

Hey there @paladin-devops - soooon! I hope to have a draft PR up this week 😄 Once it's up, it'll likely go out in the next minor Waypoint release. I can give you a ping if you're interested in checking it out in the PR form.

@paladin-devops
Copy link
Contributor

@briancain I spotted your PR, built it locally and it worked like a charm! Working on using something like Smee or Ultrahook to proxy the DockerHub webhook to Waypoint in a private network.

@briancain
Copy link
Member Author

Awesome @paladin-devops !! That's great to hear. Appreciate you trying that PR early. Very pleased to hear it worked like a charm 😄

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

Successfully merging this pull request may close these issues.

7 participants