Skip to content

Add notifyStash global function - #21

Open
mscharp wants to merge 1 commit into
jenkinsci:masterfrom
mscharp:feature/notifyStash
Open

Add notifyStash global function#21
mscharp wants to merge 1 commit into
jenkinsci:masterfrom
mscharp:feature/notifyStash

Conversation

@mscharp

@mscharp mscharp commented Apr 15, 2016

Copy link
Copy Markdown

No description provided.

@jenkinsadmin

Copy link
Copy Markdown
Member

Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests.

@ssbarnea

ssbarnea commented May 6, 2016

Copy link
Copy Markdown
Contributor

@mscharp CREDENTIAL_ID_IN_JENKINS seems like bad naming for a variable. Wouldn't be more appropiate to use something containing SLACK_TOKEN ?

@orrc

orrc commented May 6, 2016

Copy link
Copy Markdown
Member

As mentioned in the comment above that section, it's not a variable, it's a placeholder.

@ssbarnea

ssbarnea commented May 6, 2016

Copy link
Copy Markdown
Contributor

@orrc in this case is even worse, putting credentials inside the SCM is a clear no-no. Please try to find a way to keep credentials out of the code that goes into the repository.

Read http://arstechnica.co.uk/security/2016/04/hacking-slack-accounts-as-easy-as-searching-github/ to get an idea about how bad this is.

@ssbarnea

ssbarnea commented May 6, 2016

Copy link
Copy Markdown
Contributor

Also have a look at my approach at https://github.com/jenkinsci/pipeline-examples/pull/26/files -- clearly not ideal but at least it does not put the token inside the code.

I think that using environment variables or Jenkins API to access these credentials are both "good enough" security approaches. Still, inviting people to put their tokens inside Jenkinsfile, would clearly be not a good idea.

@mscharp

mscharp commented May 6, 2016

Copy link
Copy Markdown
Author

This code isn't actually putting credentials in the code. There is a "withCredentials" plugin that will use the username/password that has been set up in Jenkins. The CREDENTIAL_ID_IN_JENKINS is just a placeholder for the hash key that you access those credentials by. So, whoever uses this code would replace it with a hash string. I've attached an image of what the credentials look like in Jenkins.

screen shot 2016-05-06 at 9 04 46 am

@mscharp

mscharp commented May 6, 2016

Copy link
Copy Markdown
Author

And, you are right. There are other ways to access the credentials that you'd need. However, having it hardcoded into this script isn't a big concern for us, as the user associated with those credentials doesn't have rights to do much.

@ssbarnea

ssbarnea commented May 6, 2016

Copy link
Copy Markdown
Contributor

Ahh, sorry that would have been ok because this ID is useless outside Jenkins. I had the impression that was the slack token.

@nickbroon

Copy link
Copy Markdown

I'd guess this is a workaround until the following is implemented? jenkinsci/stashnotifier-plugin#92

@tmds

tmds commented May 27, 2016

Copy link
Copy Markdown

@mscharp how can I use this in a Jenkinsfile?

@tmds

tmds commented May 27, 2016

Copy link
Copy Markdown

@dcsobral

dcsobral commented Aug 5, 2016

Copy link
Copy Markdown

Even with the stash notifier plugin working (and supposedly it's working now), I find this is a very interesting example. Maybe make it more generic, like "calling a REST interface with build results"? The actual example can stay pretty much the same.

@kmadel

kmadel commented Aug 7, 2016

Copy link
Copy Markdown
Contributor

Note, the http-request-plugin has a Pipeline step and allows POST a request body (JSON or whatever). Furthermore, it makes it much easier to handle the response from your REST call.

@kenyee

kenyee commented Aug 20, 2016

Copy link
Copy Markdown

I was wondering how to do a POST w/ auth info securely as well, so an example of this would be helpful.

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