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

Remove Slack#server-operations notification on build success. #14753

Merged
merged 2 commits into from May 1, 2017

Conversation

ashercodeorg
Copy link
Contributor

This PR is being done as the result of team interest in making all output to Slack#server-operations actionable. Note that error output will continue to go to Slack#server-operations, as action is appropriate in this case.

Also see #14751.

aws/ci_build Outdated
@@ -45,7 +45,7 @@ def upload_log_and_get_link(key, body, metadata)
body,
{metadata: metadata}
)
" <a href='#{log_url}'> Log on S3</a>"
" <a href='#{log_url}'> Log on S3</a>"
Copy link
Contributor

Choose a reason for hiding this comment

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

No more ☁ cloud? ☁️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It goes against our styleguide of source code being in UTF8.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we swap in the HTML escape? &#9729;

Copy link
Contributor

Choose a reason for hiding this comment

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

is UTF-8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it better to use the unicode escape or the HTML escape? Note that I chose the former, though I'm happy to do the latter if more appropriate.

And, out of playful curiosity, why do we mind the cloud disappearing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted.

Copy link
Contributor

Choose a reason for hiding this comment

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

image

It's mostly for fun. I do think there's a little value in adding iconography to our Slack interface if it's not too noisy. I build subtle associations with these things (like tableflip guy when tests fail) so that, personally, I find things scan easier than if we just had a wall of text.

Copy link
Contributor

@wjordan wjordan left a comment

Choose a reason for hiding this comment

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

I don't agree that Slack#server-operations output needs be uniformly actionable (see my comment in the other PR for further discussion), just high signal-to-noise. In this case, although non-actionable, production-environment build notifications are low-volume and highly-relevant to our operations (e.g., for comparing against other error reports to determine if they occurred before/during/after a deploy), so I don't think they should be removed here.

I'm indifferent to non-production notifications (which are higher-volume and lower-relevance), so those can still be filtered if desired, as long as production notifications remain.

@ashercodeorg
Copy link
Contributor Author

Done (adding logging for production environment). PTAL.

aws/ci_build Outdated
ChatClient.log message, color: 'green'

ChatClient.message 'server operations', message, color: 'green'
ChatClient.message 'server operations', commit_url, color: 'gray', message_format: 'text'
if rack_env == 'production'
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ rack_env returns a Symbol, so this needs to be rack_env == :production or it will never be true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Done.

@ashercodeorg ashercodeorg merged commit 5ae87fb into staging May 1, 2017
@ashercodeorg ashercodeorg deleted the ciBuildOutput branch May 1, 2017 20:03
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.

None yet

3 participants