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

Post implementation for gateway #201

Merged
merged 4 commits into from
Mar 4, 2022

Conversation

samrabelachew
Copy link
Contributor

@samrabelachew samrabelachew commented Mar 2, 2022

Implements VCSPostHandler to create Gateway's version of GH event handling. It duplicates a lot of the logic/testing from the existing VCSEventsController workflow, but is different because pull request events that don't need processing (i.e. have no terraform changes) are returned immediately while requests that do are sent through SNS to the worker.

This is an alternate implementation to #191. We avoid having to modify existing CommandRunner interfaces with this approach.

case models.OpenedPullEvent, models.UpdatedPullEvent:
// If the pull request was opened or updated, we perform a pseudo-autoplan to determine if tf changes exist.
// If it exists, then we will forward request to the worker.
go g.handleOpenPullEvent(baseRepo, headRepo, pull, user, request)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just run this synchronously? Will be easier to see failure states. Previously it's asynchronous to prevent long lived connections but since we're just publishing to a queue we can remove this level of indirection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maintaining async with timer for now and will followup with move to sync processing if we find it's not expensive

server/lyft/gateway/autoplan_builder.go Outdated Show resolved Hide resolved
server/lyft/gateway/autoplan_builder.go Outdated Show resolved Hide resolved
server/lyft/gateway/autoplan_builder.go Outdated Show resolved Hide resolved
server/lyft/gateway/autoplan_builder.go Outdated Show resolved Hide resolved
server/lyft/gateway/events_controller.go Outdated Show resolved Hide resolved
server/lyft/gateway/events_controller.go Outdated Show resolved Hide resolved
server/lyft/gateway/events_controller.go Outdated Show resolved Hide resolved
server/lyft/gateway/events_controller.go Outdated Show resolved Hide resolved
server/lyft/gateway/events_controller.go Outdated Show resolved Hide resolved
PullUpdater *events.PullUpdater
}

func (r *AutoplanValidatorBuilder) IsValid(baseRepo models.Repo, headRepo models.Repo, pull models.PullRequest, user models.User) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

ValidatorBuilder 🤯 lol, how bout just validator?

Comment on lines 62 to 63
timer := r.Scope.Timer(metrics.ExecutionTimeMetric).Start()
defer timer.Stop()
Copy link
Contributor

Choose a reason for hiding this comment

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

can we move the instrumentation into a separate function? Otherwise it's large number of duplicate error calls.

return false
}
if r.DisableAutoplan {
r.Scope.Counter(metrics.ExecutionErrorMetric).Inc(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is weird to me, it shouldn't be an error because a flag is set. Let's remove this, we don't even use this code path anyways.

Samra Belachew added 2 commits March 4, 2022 14:06
@samrabelachew samrabelachew merged commit 70ffa5f into release-v0.17.3-lyft.1 Mar 4, 2022
@samrabelachew samrabelachew deleted the gateway-event-handler branch March 4, 2022 23:36
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.

2 participants