Feature/configure email notifications #10
Conversation
|
Hey there, Great to see you opening issues and pull requests. Hopefully we can knock some of these out. I will take a look at this in the morning and provide any feedback that I have. |
manifests/init.pp
Outdated
| String $smtp_sender_displayname = $wsusserver::params::smtp_sender_displayname, | ||
| String $smtp_sender_emailaddress = $wsusserver::params::smtp_sender_emailaddress, | ||
| String $email_language = $wsusserver::params::email_language, | ||
|
|
There was a problem hiding this comment.
Still waiting on you to do this.
| @@ -19,6 +19,20 @@ | |||
| Boolean $synchronize_automatically = $wsusserver::params::synchronize_automatically, | |||
| String $synchronize_time_of_day = $wsusserver::params::synchronize_time_of_day, | |||
| Integer $number_of_synchronizations_per_day = $wsusserver::params::number_of_synchronizations_per_day, | |||
| Boolean $send_sync_notification = $wsusserver::params::send_sync_notification, | |||
There was a problem hiding this comment.
Can you document these parameters in the README.md ( https://github.com/TraGicCode/tragiccode-wsusserver/blob/master/README.md#wsusserver-1 ) like the other parameters that make up the public interface to this class?
There was a problem hiding this comment.
Still waiting on you to do this.
|
Also if you can please squash you commits to clean up some of the noise with the WIP and oops commits. |
|
Hi I made review changes Regards |
|
This should handle the squash for you. git checkout feature/configure-emailnotifications
git reset $(git merge-base master feature/configure-emailnotifications)
git add -A
git commit -m "Added email feature or something like this"
git push <your_remote_name> HEAD --force |
jblance
force-pushed
the
feature/configure-emailnotifications
branch
from
47f03e4
to
b98dd0b
Compare
|
Thanks for the squash help |
|
@jblance The squash looks good to me. Can you fix the above changes i requested? Once done simply type "Done." in each comment. |
manifests/config.pp
Outdated
| \$wsusEmailNotificationConfiguration = (Get-WsusServer).GetEmailNotificationConfiguration() | ||
| \$wsusEmailNotificationConfiguration.SendSyncNotification = \$${send_sync_notification} | ||
| \$wsusEmailNotificationConfiguration.Save() | ||
| return 'SendSyncNotification setting updated' |
There was a problem hiding this comment.
Can we remove this? Doubt it's needed.
There was a problem hiding this comment.
Remove which bit?
This sets the "Send email on sync update", i.e turning the feature on or off
I think this is needed, unless I'm missing something
There was a problem hiding this comment.
I'm referring to the return 'SendSyncNotification setting updated' piece since it is not necessary
There was a problem hiding this comment.
Oh OK - I've removed them
I did quite like that it output that info when running puppet agent -tv manually
manifests/config.pp
Outdated
| \$wsusEmailNotificationConfiguration = (Get-WsusServer).GetEmailNotificationConfiguration() | ||
| \$wsusEmailNotificationConfiguration.SendStatusNotification = \$${send_status_notification} | ||
| \$wsusEmailNotificationConfiguration.Save() | ||
| return 'SendStatusNotification setting updated' |
|
@jblance Here are some more minor changes. I will run setup some beaker tests and perform some acceptance testing and let you know of any issues i run into. |
|
@jblance Thank you for your contribution. Hope to see more again soon! |
My initial attempt to implement email notification configuring (for issue #8 )
Note: authenticated SMTP is not implemented
Note2: no spec tests as I have figured them out yet....