Orca 447 add server config to run pre hooks #8

msarvar · 2020-10-15T17:56:52Z

Adding a run step that executes before any workflow steps.
runatlantis/atlantis#500 original issue that requested the feature.

msarvar · 2020-10-19T16:38:02Z

/ptal #orchestration @nishkrishnan @paulnivin

nishkrishnan · 2020-10-19T19:04:20Z

can you rebase? It should pull in the gh action that I pushed

msarvar · 2020-10-19T20:57:34Z

/ptal

nishkrishnan

Nice job! Have some comments/questions

nishkrishnan · 2020-10-19T20:59:47Z

server/events/models/models.go

+	// be the same as BaseRepo.
+	HeadRepo Repo
+	// Log is a logger that's been set up for this context.
+	Log *logging.SimpleLogger


Should we be passing the interface here?

nishkrishnan · 2020-10-19T22:51:43Z

server/events/yaml/raw/global_cfg.go

-	AllowCustomWorkflows *bool    `yaml:"allow_custom_workflows,omitempty" json:"allow_custom_workflows,omitempty"`
+	ID                   string         `yaml:"id" json:"id"`
+	ApplyRequirements    []string       `yaml:"apply_requirements" json:"apply_requirements"`
+	WorkflowHooks        []WorkflowHook `yaml:"pre_workflow_hooks" json:"pre_workflow_hooks"`


I would keep the naming consistent and just call this PreWorkflowHooks

nishkrishnan · 2020-10-19T22:53:27Z

server/events/workflow_hooks_command_result.go

+// WorkflowHooksCommandResult is the result of executing pre workflow hooks for a
+// repository.
+type WorkflowHooksCommandResult struct {
+	WorkflowHookResults []models.WorkflowHookResult


Curious as to why you chose to support a list at this point? I don't think we have a need for it right? and even if we did, might be easier to to just have all the steps listed in the external script itself. Plus it seems that this isn't really following the normal go convention of return some value + an error which feels a bit weird in a communal code base.

You're right it is not needed here. I will remove it. It is used for command_runner where you have multiple projects running in parallel.

nishkrishnan · 2020-10-19T22:54:24Z

server/events/workflow_hooks_command_runner.go

+		log.Warn("workspace is locked")
+		return
+	}
+	log.Debug("got workspace lock")


if you're gonna leave this in maybe add a bit more like the root name/repo name

Same with the above

this log is an instance of a logger and it uses repo_name#pr_num as a prefix.
This is an example from my local

2020/10/19 16:43:53-0700 [DBUG] pre_workflow_hooks_command_runner.go:61 msarvar/atlantis-local-dev#28: Got workspace lock

I can't do better than repo_name and PR since I don't have root information at this point.

nishkrishnan · 2020-10-19T22:56:43Z

server/events/workflow_hooks_command_runner.go

+		headRepo models.Repo,
+		pull models.PullRequest,
+		user models.User,
+	)


Why aren't we returning an error at least in the interface?

Initially it was returning an object and error but CommandRunner interface doesn't have any return values. So decided to be consistent.

Hmm I see yeah ok we can probably be consistent then.

nishkrishnan · 2020-10-19T22:59:17Z

server/events/workflow_hooks_command_runner.go

+		Verbose:  false,
+	}
+
+	result := w.runHooks(ctx, workflowHooks, repoDir)


Seems odd to me to have an object called result that doesn't get returned and is only used as an error check.

Couldn't we just as easily return the first error we get in the function and check here whether that is nil or not?

Yeah, i was trying too hard to follow the atlantis implementation for run_steps. But it makes sense to reduce the wrapping.

nishkrishnan · 2020-10-19T22:59:52Z

server/events/workflow_hooks_command_runner.go

+	if err := recover(); err != nil {
+		stack := recovery.Stack(3)
+		logger.Err("PANIC: %s\n%s", err, stack)
+		if commentErr := w.VCSClient.CreateComment(


Is there precendence for logging panics to the PR elsewhere in the repo?

Seems like a bad user experience to do that but don't want to deviate from the standard if that's set already.

Yep, it is in the command_runner.go both RunCommentCommand and RunAutoplanCommand use it. What would alternative be? Potentially we can log and move on for pre_workflow_hooks.

It's a good question.

i think in this case, the hooks are going to be only configured server side right? So customers shouldn't really know about it. Hence I would lean towards something like we just log an error and a stat (and alarm on this internally) and just move on.

In our case, there will probably be some downstream error about missing an atlantis configuration file so it's probably ok. In the general case, I'm not sure what people would use this for, but might be safer to not panic on failure in case it's something non-critical.

What do you think?

I think that should work for our use case.

nishkrishnan · 2020-10-19T23:04:41Z

server/events_controller.go

+	// TODO: run pre workflow hooks
+	// e.Logger.Info("running pre workflow hooks")
+	// e.WorkflowHooksCommandRunner.RunPreHooks(baseRepo, headRepo, pull, user)
+


nishkrishnan · 2020-10-19T23:05:21Z

server/server.go

+		AtlantisURL:                parsedURL,
+		Router:                     underlyingRouter,
+		Port:                       userConfig.Port,
+		WorkflowHooksCommandRunner: workflowHooksCommandRunner,


PreWorkflowHooksCommandRunner*

nishkrishnan

🚢

nishkrishnan · 2020-10-20T00:07:22Z

server/events/yaml/raw/global_cfg.go

-	AllowCustomWorkflows *bool    `yaml:"allow_custom_workflows,omitempty" json:"allow_custom_workflows,omitempty"`
+	ID                   string         `yaml:"id" json:"id"`
+	ApplyRequirements    []string       `yaml:"apply_requirements" json:"apply_requirements"`
+	PreWorkflowHooks        []PreWorkflowHook `yaml:"pre_workflow_hooks" json:"pre_workflow_hooks"`


I'm assuming we don't support this in the repo config currently?

Not sure what would be the use case for repo config? For repo config there is a custom run commands.

msarvar · 2020-10-20T00:32:32Z

💨

msarvar force-pushed the ORCA-447-add-server-config-to-run-pre-hooks branch 2 times, most recently from b6b69ad to 0c5c20e Compare October 19, 2020 19:26

msarvar added 9 commits October 19, 2020 12:28

Adding PreWorkflowHooks into global config.

4a96d67

Implemented WorkflowHooksCommand to run on RP open

8c0f56c

POC finished for running an arbitrary script before running autoplan

5d9ffca

Wrapping pre workflow hooks into a struct

b693bbe

update interface

1d42d47

Refactoring some names and adding some tests

d69fa80

Adding tests to workflow_hook_runner

b6424ee

Test additions

62c9322

Removing unnecessary code

1223315

msarvar force-pushed the ORCA-447-add-server-config-to-run-pre-hooks branch from 0c5c20e to 1223315 Compare October 19, 2020 19:29

msarvar added 6 commits October 19, 2020 12:45

Fixing tests

4153ca8

fixed yaml/valid test

e66a4d0

Test fix

30a23a0

Linting fix

fa13549

Removed return value from RunPreHooks

de76aa6

linting fix

b40e9e7

nishkrishnan reviewed Oct 19, 2020

View reviewed changes

Addressing PR comments.

556d70c

msarvar force-pushed the ORCA-447-add-server-config-to-run-pre-hooks branch from 3f93fdf to 556d70c Compare October 20, 2020 00:00

msarvar added 2 commits October 19, 2020 17:05

Fixing tests

203bcf0

Linting and interface update

7c748af

msarvar force-pushed the ORCA-447-add-server-config-to-run-pre-hooks branch from 931368c to 7c748af Compare October 20, 2020 00:28

nishkrishnan approved these changes Oct 20, 2020

View reviewed changes

msarvar merged commit 2e74997 into release-v0.15.0-lyft.1 Oct 20, 2020

msarvar deleted the ORCA-447-add-server-config-to-run-pre-hooks branch October 20, 2020 00:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Orca 447 add server config to run pre hooks #8

Orca 447 add server config to run pre hooks #8

msarvar commented Oct 15, 2020 •

edited

Loading

msarvar commented Oct 19, 2020

nishkrishnan commented Oct 19, 2020

msarvar commented Oct 19, 2020

nishkrishnan left a comment

nishkrishnan Oct 19, 2020

nishkrishnan Oct 19, 2020

nishkrishnan Oct 19, 2020

msarvar Oct 19, 2020

nishkrishnan Oct 19, 2020

nishkrishnan Oct 19, 2020

msarvar Oct 19, 2020

msarvar Oct 19, 2020

nishkrishnan Oct 19, 2020

msarvar Oct 19, 2020

nishkrishnan Oct 19, 2020

nishkrishnan Oct 19, 2020

msarvar Oct 19, 2020

nishkrishnan Oct 19, 2020

nishkrishnan Oct 19, 2020

msarvar Oct 19, 2020

nishkrishnan Oct 19, 2020

msarvar Oct 20, 2020

nishkrishnan Oct 19, 2020

nishkrishnan Oct 19, 2020

nishkrishnan left a comment

nishkrishnan Oct 20, 2020

msarvar Oct 20, 2020

msarvar commented Oct 20, 2020

Orca 447 add server config to run pre hooks #8

Orca 447 add server config to run pre hooks #8

Conversation

msarvar commented Oct 15, 2020 • edited Loading

msarvar commented Oct 19, 2020

nishkrishnan commented Oct 19, 2020

msarvar commented Oct 19, 2020

nishkrishnan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nishkrishnan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msarvar commented Oct 20, 2020

msarvar commented Oct 15, 2020 •

edited

Loading