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

feat: Add ability to set pr status context with --status-name flag #841

Closed

Conversation

js-timbirkett
Copy link
Contributor

@js-timbirkett js-timbirkett commented Nov 14, 2019

Hi 👋

Background

I recently commented on #249 with regards to using multiple Atlantis servers against a single repo.

I have a reasonably good setup with an Atlantis server running in a "dev" account and one in a "prd" account (due to no access to a management account or ability to allow cross-account trusted roles). This works well, I can set my dev server to allow applying mergable changes and my prd server allows applying only approved changes with automerging.

The Problem

The PR status gets updated with "atlantis/{plan, apply, ...}" from both servers which makes it difficult to know which server had issues with, or is running a plan / apply and can cause some confusion.

The Solution

A --status-name allows users to override the name used in PR status contexts per server.

Example --status-name atlantis-noprod results in:
Screenshot 2019-11-14 at 11 05 46 AM

As always, feedback or recommendation is greatly welcome and appreciated!

Thanks,

Tim

@codecov
Copy link

codecov bot commented Nov 14, 2019

Codecov Report

Merging #841 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #841      +/-   ##
==========================================
+ Coverage   71.83%   71.85%   +0.01%     
==========================================
  Files          65       65              
  Lines        5213     5215       +2     
==========================================
+ Hits         3745     3747       +2     
  Misses       1183     1183              
  Partials      285      285
Impacted Files Coverage Δ
server/events/runtime/env_step_runner.go 100% <ø> (ø) ⬆️
server/user_config.go 100% <ø> (ø) ⬆️
cmd/server.go 78.97% <100%> (+0.19%) ⬆️
server/events/commit_status_updater.go 100% <100%> (ø) ⬆️
server/server.go 63.58% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bef2c09...c670c85. Read the comment docs.

Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

Looks great! Just a couple small nits.

@@ -72,6 +72,7 @@ const (
SlackTokenFlag = "slack-token"
SSLCertFileFlag = "ssl-cert-file"
SSLKeyFileFlag = "ssl-key-file"
StatusName = "status-name"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
StatusName = "status-name"
PRStatusName = "pr-status-name"

I think adding PR will save some confusion as to what status we're talking about.

@@ -87,6 +88,7 @@ const (
DefaultLogLevel = "info"
DefaultPort = 4141
DefaultTFEHostname = "app.terraform.io"
DefaultStatusName = "atlantis"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
DefaultStatusName = "atlantis"
DefaultPRStatusName = "atlantis"

@@ -719,6 +720,7 @@ write-git-creds: true
"TFE_HOSTNAME": "override-my-hostname",
"TFE_TOKEN": "override-my-token",
"WRITE_GIT_CREDS": "false",
Copy link
Member

Choose a reason for hiding this comment

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

Missed adding TestExecute_Flags, TestExecute_ConfigFile

@@ -762,6 +764,7 @@ write-git-creds: true
Equals(t, "override-my-hostname", passedConfig.TFEHostname)
Equals(t, "override-my-token", passedConfig.TFEToken)
Equals(t, false, passedConfig.WriteGitCreds)
Equals(t, "override-status-name", passedConfig.StatusName)
Copy link
Member

Choose a reason for hiding this comment

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

You also need to add status-name: status to line 679 to match the rest of the test

@@ -39,11 +39,12 @@ type CommitStatusUpdater interface {

// DefaultCommitStatusUpdater implements CommitStatusUpdater.
type DefaultCommitStatusUpdater struct {
Client vcs.Client
Client vcs.Client
StatusName string
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
StatusName string
// PRStatusName is the name used when updating the PR status.
PRStatusName string

"os"
"testing"

"github.com/hashicorp/go-version"
"github.com/runatlantis/atlantis/server/events/runtime"

Copy link
Member

Choose a reason for hiding this comment

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

extra newline

@@ -44,6 +44,7 @@ type UserConfig struct {
SlackToken string `mapstructure:"slack-token"`
SSLCertFile string `mapstructure:"ssl-cert-file"`
SSLKeyFile string `mapstructure:"ssl-key-file"`
StatusName string `mapstructure:"status-name"`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
StatusName string `mapstructure:"status-name"`
PRStatusName string `mapstructure:"pr-status-name"`

@lkysow lkysow mentioned this pull request Jan 20, 2020
@lkysow lkysow closed this in #904 Jan 20, 2020
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