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

Include directory in slack webhook message #662

Merged
merged 2 commits into from
Jun 5, 2019
Merged

Include directory in slack webhook message #662

merged 2 commits into from
Jun 5, 2019

Conversation

tfheen
Copy link

@tfheen tfheen commented Jun 4, 2019

Fixes: #660

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.

  • tests are failing, looks like you need to change what are the expected calls on the mock
  • can you provide a screenshot of what the notification looks like now

@codecov
Copy link

codecov bot commented Jun 5, 2019

Codecov Report

Merging #662 into master will increase coverage by <.01%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #662      +/-   ##
=========================================
+ Coverage    72.2%   72.2%   +<.01%     
=========================================
  Files          61      61              
  Lines        4644    4652       +8     
=========================================
+ Hits         3353    3359       +6     
- Misses       1045    1046       +1     
- Partials      246     247       +1
Impacted Files Coverage Δ
server/events/webhooks/webhooks.go 73.33% <ø> (ø) ⬆️
server/events/project_command_runner.go 72.89% <100%> (+0.25%) ⬆️
server/events/webhooks/slack_client.go 96.29% <71.42%> (-3.71%) ⬇️

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 1fa621d...4944ee1. Read the comment docs.

@tfheen
Copy link
Author

tfheen commented Jun 5, 2019

Output:
image

Arguably, it should only be printed if directory is non-empty, happy to make that change if you'd like that.

@lkysow
Copy link
Member

lkysow commented Jun 5, 2019

I think it's okay to always print the directory because it would be weird if you had

/
  main.tf
  subproject/
    main.tf

And it only printed the directory for subproject.

The directory is relative and if it's in the root, it will just be .. Let's detect that and use / in that case.

@tfheen
Copy link
Author

tfheen commented Jun 5, 2019

Updated to make . print as /.

@@ -98,6 +98,11 @@ func (d *DefaultSlackClient) createAttachments(applyResult ApplyResult) []slack.
}

text := fmt.Sprintf("Apply %s for <%s|%s>", successWord, applyResult.Pull.URL, applyResult.Repo.FullName)
directory := applyResult.Directory
if directory == "." {
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
if directory == "." {
// Since "." looks weird, replace it with "/" to make it clear this is the root.
if directory == "." {

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.

Just one small change please. Then looks good.

@lkysow lkysow merged commit 708fa0c into runatlantis:master Jun 5, 2019
@lkysow
Copy link
Member

lkysow commented Jun 5, 2019

I realized I could make the edit myself, thanks!

@tfheen
Copy link
Author

tfheen commented Jun 7, 2019

Thanks for merging!

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.

Enrich Slack messages
2 participants