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

Implement notifications configuration for .travis.yml #136

Merged
merged 3 commits into from
Nov 27, 2018

Conversation

nmaludy
Copy link
Contributor

@nmaludy nmaludy commented Sep 4, 2018

Closes #119

This PR implements an override in .travis.yml for the notifications section so repos can send notifications to things like Slack.

@ardrigh
Copy link
Contributor

ardrigh commented Oct 18, 2018

I have need for similar functions. It would be nice if these PRs could be reviewed.

Copy link
Contributor

@scotje scotje left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @nmaludy !

I added a couple of stylistic suggestions but feel free to provide rationale for the current choices if you feel they are merited.

@@ -88,7 +88,18 @@ branches:
<% end -%>
<% end -%>
notifications:
email: false
<% notifications_arr = ((@configs['notifications'].to_a || []) - (@configs['remove_notifications'].to_a || [])) -%>
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to guard against @configs['notifications'] possibly being nil? I realize it has a default, but someone maybe could override it to nil?

Copy link
Contributor

Choose a reason for hiding this comment

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

Was there a situation where this value was not an array but could be safely coerced into an array? If not, we can just drop to:

Suggested change
<% notifications_arr = ((@configs['notifications'].to_a || []) - (@configs['remove_notifications'].to_a || [])) -%>
<% notifications_arr = ((@configs['notifications'] || []) - (@configs['remove_notifications'] || [])) -%>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@scotje Notifications is a hash, but in order to provide a "difference" between the @config['notifications'] and @config['remove_notifications'] i converted to an array to make this difference logic easier.

moduleroot/.travis.yml.erb Outdated Show resolved Hide resolved
@bmjen bmjen merged commit 3113f4e into puppetlabs:master Nov 27, 2018
@ardrigh
Copy link
Contributor

ardrigh commented Jan 30, 2019

I think this code creates a bug in .travis.yml if you need to set email: false.

In .sync.yml this adds the slack channel notification and removes email:false in .travis.yml

  notifications:
    - slack:
        secure: BLAHBLAHBLAHBLAHBLAHBLAH

I tried a few variations of notifications but none of them add email:false as expected.

Any ideas?

@scotje
Copy link
Contributor

scotje commented Mar 19, 2019

@ardrigh I was able to configure both email: false and a slack notification with the following in my .sync.yml:

notifications:
  email: false
  slack:
    secure: foozle

and running pdk update. Does that config work for you? If not, could you open a new issue here or on the puppetlabs/pdk project?

@ardrigh
Copy link
Contributor

ardrigh commented Mar 19, 2019

@scotje sorry I haven't tested this for a while.

I just tried PDK 1.9.1 and I have the format working now, so it's not trying to remove the secure notification for Slack any longer.

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.

4 participants