Adding in slack-token to flags #428

jacobfoard · 2019-01-15T19:29:40Z

In my testing of the slack functionality, I found I was able to specify the token via the config.yaml, but the env var/flag wasn't working. This allow env/flag/config to work as the rest of the flags do.

Signed-off-by: Jacob Foard jacob.foard@formation.ai

codecov · 2019-01-15T22:00:56Z

Codecov Report

Merging #428 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #428   +/-   ##
=======================================
  Coverage   70.78%   70.78%           
=======================================
  Files          63       63           
  Lines        3987     3987           
=======================================
  Hits         2822     2822           
  Misses        970      970           
  Partials      195      195

Impacted Files	Coverage Δ
cmd/server.go	`78.94% <ø> (ø)`	⬆️

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 c0b89d9...142c6a2. Read the comment docs.

cmd/server.go

jacobfoard · 2019-01-15T22:02:31Z

cmd/server_test.go

@@ -565,7 +570,8 @@ repo-whitelist: "github.com/runatlantis/atlantis"
 require-approval: true
 ssl-cert-file: cert-file
 ssl-key-file: key-file
-ssl-key-file: my-token
+tfe-token: my-token


I assume this was a botched copy paste, and updated it accordingly

good catch! 🍺

lkysow · 2019-01-17T18:06:38Z

Thanks for the pull request! Please rebase off of master since you've got some changes deleting tfe-token stuff that should not be in here.

Also please fix your vscode import changes.

cmd/server.go

lkysow · 2019-01-17T18:11:44Z

cmd/server.go

@@ -174,6 +175,10 @@ var stringFlags = []stringFlag{
 			" Only set if using TFE as a backend." +
 			" Should be specified via the ATLANTIS_TFE_TOKEN environment variable for security.",
 	},
+	{
+		name:        SlackTokenFlag,
+		description: "API token required for Slack webhook integration",


What kind of API token is required? I forget.

I wouldn't say "slack webhook" because it's a slack notification, not a webhook.

This is a "bot" token, and I've reworded it. Probably worth putting in an another PR for #42 though.

lkysow · 2019-01-17T19:56:58Z

I think you need to rebase? The PR has 15 commits now and is showing changes that are already on master.

You should have one commit and the GitHub UI should show just your changes.

jacobfoard · 2019-01-17T19:58:16Z

Yea, I rebased in the cli not sure what's the issue. I'll get it fixed shortly.

jacobfoard · 2019-01-17T22:46:48Z

Alright, sorry about that. Learned a lot about git today. I've rebased and reordered.

sphvn · 2019-01-17T22:49:13Z

cmd/server_test.go

 	Equals(t, "override-cert-file", passedConfig.SSLCertFile)
 	Equals(t, "override-key-file", passedConfig.SSLKeyFile)
 	Equals(t, "override-my-token", passedConfig.TFEToken)
+


This newline required?

I think not, please remove.

Suggested change

lkysow · 2019-01-22T22:09:48Z

cmd/server.go

@@ -22,7 +22,7 @@ import (

 	"github.com/runatlantis/atlantis/server/logging"

-	"github.com/mitchellh/go-homedir"
+	homedir "github.com/mitchellh/go-homedir"


Please don't make this change.

Suggested change

homedir "github.com/mitchellh/go-homedir"

"github.com/mitchellh/go-homedir"

lkysow

Glad you figured out your git issues, those can be thorny sometimes (https://xkcd.com/1597/).

I've got a couple comments I'd like addressed.

lkysow · 2019-01-22T22:12:48Z

cmd/server_test.go

@@ -565,7 +570,8 @@ repo-whitelist: "github.com/runatlantis/atlantis"
 require-approval: true
 ssl-cert-file: cert-file
 ssl-key-file: key-file
-ssl-key-file: my-token
+tfe-token: my-token


good catch! 🍺

lkysow · 2019-01-22T22:13:06Z

cmd/server_test.go

 	Equals(t, "override-cert-file", passedConfig.SSLCertFile)
 	Equals(t, "override-key-file", passedConfig.SSLKeyFile)
 	Equals(t, "override-my-token", passedConfig.TFEToken)
+


I think not, please remove.

lkysow · 2019-01-22T22:16:59Z

cmd/server.go

@@ -172,6 +173,10 @@ var stringFlags = []stringFlag{
 			"all repos: '*' (not recommended), an entire hostname: 'internalgithub.com/*' or an organization: 'github.com/runatlantis/*'." +
 			" For Bitbucket Server, {hostname} is the domain without scheme and port, {owner} is the name of the project (not the key), and {repo} is the repo name.",
 	},
+	{
+		name:        SlackTokenFlag,
+		description: "API token required for Slack notification integration",


Can this read:

Suggested change

description: "API token required for Slack notification integration",

description: "API token for Slack notifications.",

lkysow · 2019-01-22T22:18:39Z

cmd/server_test.go

 	Equals(t, "override-cert-file", passedConfig.SSLCertFile)
 	Equals(t, "override-key-file", passedConfig.SSLKeyFile)
 	Equals(t, "override-my-token", passedConfig.TFEToken)
+


Suggested change

lkysow · 2019-01-22T22:19:42Z

I would suggest a rebase again 😄

jacobfoard · 2019-01-22T22:27:37Z

I think I've gotten all of the fixes in place, give me a few to battle git again and I'll get it rebased.

Signed-off-by: Jacob Foard <jacob.foard@formation.ai> Adding in tests, fixing tfe-override for one test Signed-off-by: Jacob Foard <jacob.foard@formation.ai> Alphabetizing config + config tests, removing homedir alias Signed-off-by: Jacob Foard <jacob.foard@formation.ai> Reordering Signed-off-by: Jacob Foard <jacob.foard@formation.ai> Fixing homedir alias Signed-off-by: Jacob Foard <jacob.foard@formation.ai> Fixing slack flag wording, and new lines. With vim this time so I don't have to keep editing the homedir alias Signed-off-by: Jacob Foard <jacob.foard@formation.ai>

lkysow · 2019-01-23T03:33:04Z

Thanks! I've merged this via #442.

jacobfoard commented Jan 15, 2019

View reviewed changes

cmd/server.go Outdated Show resolved Hide resolved

jacobfoard commented Jan 15, 2019

View reviewed changes

lkysow requested changes Jan 17, 2019

View reviewed changes

sphvn reviewed Jan 17, 2019

View reviewed changes

lkysow reviewed Jan 22, 2019

View reviewed changes

Jacob Foard and others added 2 commits January 22, 2019 14:51

Add missing period to slack-token flag docs

142c6a2

lkysow mentioned this pull request Jan 23, 2019

Add slack-token to flags. #442

Merged

lkysow closed this in #442 Jan 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding in slack-token to flags #428

Adding in slack-token to flags #428

jacobfoard commented Jan 15, 2019

codecov bot commented Jan 15, 2019 •

edited

jacobfoard Jan 15, 2019

lkysow Jan 22, 2019

lkysow commented Jan 17, 2019

lkysow Jan 17, 2019

jacobfoard Jan 17, 2019

lkysow commented Jan 17, 2019

jacobfoard commented Jan 17, 2019

jacobfoard commented Jan 17, 2019

sphvn Jan 17, 2019

lkysow Jan 22, 2019

lkysow Jan 22, 2019

lkysow Jan 22, 2019 •

edited

lkysow left a comment

lkysow Jan 22, 2019

lkysow Jan 22, 2019

lkysow Jan 22, 2019

lkysow Jan 22, 2019

lkysow commented Jan 22, 2019

jacobfoard commented Jan 22, 2019

lkysow commented Jan 23, 2019

	homedir "github.com/mitchellh/go-homedir"
	"github.com/mitchellh/go-homedir"

	description: "API token required for Slack notification integration",
	description: "API token for Slack notifications.",

Adding in slack-token to flags #428

Adding in slack-token to flags #428

Conversation

jacobfoard commented Jan 15, 2019

codecov bot commented Jan 15, 2019 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkysow commented Jan 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkysow commented Jan 17, 2019

jacobfoard commented Jan 17, 2019

jacobfoard commented Jan 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkysow Jan 22, 2019 • edited

Choose a reason for hiding this comment

lkysow 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

lkysow commented Jan 22, 2019

jacobfoard commented Jan 22, 2019

lkysow commented Jan 23, 2019

codecov bot commented Jan 15, 2019 •

edited

lkysow Jan 22, 2019 •

edited