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

Create proxy command runner to toggle between platform mode worklow #197

Merged

Conversation

msarvar
Copy link

@msarvar msarvar commented Feb 28, 2022

Created a PlatformModeFeatureRunner that acts as a proxy for plan, apply and policy check command runners.
Created dummy plan and apply runners that only comment to github

}

func (r *Runner) Run(ctx *command.Context, cmd *events.CommentCommand) {
if err := r.vcsClient.CreateComment(ctx.Pull.BaseRepo, ctx.Pull.Num, "I'm a platform mode approve_policies runner", command.ApprovePolicies.String()); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This description lmao.

Comment on lines 31 to 35
type featureRunnerFunc func(ctx *command.Context, cmd *events.CommentCommand)

func (f featureRunnerFunc) Run(ctx *command.Context, cmd *events.CommentCommand) {
f(ctx, cmd)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this necessary vs. just an interface? All the methods seem to have the same signature? Is there a reason we can't just use a decorator?

Copy link
Author

Choose a reason for hiding this comment

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

This is to avoid instantiating a new decorator instance for each command. For instance in server.go I'm doing this:

	featuredRunner := lyftCommands.NewPlatformModeFeatureRunner(
		featureAllocator,
		userConfig.EnablePlatformMode,
		logger,
	)

	commentCommandRunnerByCmd := map[command.Name]comment.Runner{
		command.Plan: featuredRunner.Wrap(
			plan.NewRunner(vcsClient),
			planCommandRunner,
		),
		command.Apply: featuredRunner.Wrap(
			apply.NewRunner(vcsClient),
			applyCommandRunner,
		),
		command.ApprovePolicies: featuredRunner.Wrap(
			policies.NewRunner(vcsClient),
			approvePoliciesCommandRunner,
		)
	}

Instead of initializing a new runner for each plan. This is basically a decorator pattern as well, just more functional inspired. Based on this talk. I also fixed the signature of the Wrap function to return CommentCommandRunner interface.

Copy link
Author

Choose a reason for hiding this comment

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

I changed the code to use decorator struct.

return
}

shouldAllocate, err := r.featureAllocator.ShouldAllocate(feature.PlatformMode, ctx.HeadRepo.FullName)
Copy link
Contributor

Choose a reason for hiding this comment

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

not in this PR but something we should do is emit stats within the feature allocator when there is an error.

nishkrishnan
nishkrishnan previously approved these changes Mar 4, 2022
@msarvar msarvar merged commit 413f776 into release-v0.17.3-lyft.1 Mar 5, 2022
@msarvar msarvar deleted the orca-4469-create-proxy-command-runner branch March 5, 2022 00:04
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