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

Slack notice #49831

Closed
wants to merge 41 commits into from
Closed

Slack notice #49831

wants to merge 41 commits into from

Conversation

dubb-b
Copy link

@dubb-b dubb-b commented Sep 29, 2018

What does this PR do?

Adds slack notice to failed PR's in channel #jenkins-prod-pr

What issues does this PR fix or reference?

New Feature

Previous Behavior

None

New Behavior

Awesome notifications now!!!

Commits signed with GPG?

Yes

twangboy and others added 19 commits September 21, 2018 09:32
Fix docs formatting
Clean up code
Use explicit function calls
Add comment about reboot status
Execution Module:
- Passing no DNS servers will clear the list.
- Update documentation
- Added a note about clearing DNS entries

State Module
- An empty list in dns_servers will clear the entires
- None in dns_servers will do nothing (default behavior)
- Update Documentation
Add test=True support when clearing
Use named parameters where possible
The cache files used for these are based on the state's tag, so longer
ID decs or names will cause errors when the filename's length exceeds
the max allowed by the kernel.

This fixes these errors by taking (up to) the first 32 chars of the
result of base64-encoding the tag, and using that as the parallel cache
filename.
The setUp only needs to run at the beginning so it can be a setUpClass,
and the tearDown logic only applied to a single test and should not be
run after each test.
The code works without this because at least one of the other modules
being imported also imports this. But explicitly importing it here will
keep us from getting bitten if the imports change elsewhere later on.
@damon-atkins
Copy link
Contributor

damon-atkins commented Sep 29, 2018

Try this out, but you will need to write the code I assume. Given all tests need a working python you could use python to pull the info out of github.
https://api.github.com/repos/SaltStack/Salt/pulls/49831
Which will give you url for each reviewer "url": "https://api.github.com/users/gtmanfred",
Then in the message assume the github username and slack are the same and use @dubb-b in the slack message. But you might want to only include reviews when the tests past.

@dubb-b
Copy link
Author

dubb-b commented Sep 29, 2018

@damon-atkins Are you in community slack? If so hit me up, lets talk more about this. I am just trying to get a slack message for any PR that fails into a slack message so that we can start to identify them. But this is just a stop gap for now. We want to add some sort of TICK stack to Jenkins so that we get metrics on all builds, branch, PR etc.. Then we can group the data better. But I would love to hear any ideas you have. I've noticed you comment about this before. This process is one of our top priorities. And any feedback is welcome.

In your suggestion for improving this do you think adding more data is relevant? I could have a script that pulls the reviewer into the message. And quite a bit more if needed. @rallytime would this help at all?

I am checking out for the eve, although Monday feel free to hit me up.

Nicole Thomas and others added 4 commits September 30, 2018 07:17
@rallytime
Copy link
Contributor

rallytime commented Sep 30, 2018

Then in the message assume the github username and slack are the same

We can't really assume that. There's lots of people who don't have the same GitHub and Slack usernames.

@dubb-b It seems here that you've got more commits in this than intended - this looks like you've got the stuff that has already been merged in here (i.e. the timeouts for docs/lint) rather than just the slack additions. Maybe this needs a rebase? It makes these kinds of changes a lot easier to review when the commit list is paired down to the relevant changes. :)

We can definitely chat more about this on Monday, but I think the general implementation is a great place to start.

Copy link
Contributor

@gtmanfred gtmanfred left a comment

Choose a reason for hiding this comment

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

A few cosmetic changes.

.ci/docs Outdated
@@ -45,6 +48,7 @@ pipeline {
description: 'The docs job has failed',
status: 'FAILURE',
context: "jenkins/pr/docs"
slackSend (channel: "#jenkins-prod-pr", color: '#FF0000', message: "FAILED: PR-Job: '${env.JOB_NAME} [${env.BUILD_NUMBER}]' (${env.BUILD_URL})" + "\n It's not denial. I'm just selective about the reality I accept.")
Copy link
Contributor

Choose a reason for hiding this comment

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

this indent looks wrong.

terminalmage and others added 3 commits October 1, 2018 08:58
A space here will cause the Sphinx role to treat whatever is after the
comma as the comment for the directive, so it will be rendered with
different CSS and look weird. Also, this fixes one syntactically
incorrect versionadded which included a typo.
Fix badly formatted versionadded directive (2017.7 branch)
Use utf-8 encoding for salt-runtests.log
@dubb-b
Copy link
Author

dubb-b commented Oct 1, 2018

@rallytime This is odd, it grabbed all the other stuff as well. This is a new branch and all I wanted was the slack changes here. So i will close this and re-open a new one with the right commits.

@gtmanfred
Copy link
Contributor

do a git pull --rebase origin 2017.7

Copy link
Contributor

@rallytime rallytime left a comment

Choose a reason for hiding this comment

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

Once this is rebased I can review this.

Paulo-Nunes and others added 11 commits October 1, 2018 21:08
This should actually cause builds to fail when any part in the try block
fails. We still delete instances and try junit rendering, but if things
fail then they get marked on github as failed.
Actually catch the exception when we fail
Better support for ec2 on laptops that also have vagrant installed.
Revert this after there is an upstream release
Populate changes dictionary on system.join_domain
Remove arrInstalled artifact
@rallytime
Copy link
Contributor

@dubb-b Where are we with this one?

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.

9 participants