Skip to content

remove ssl-disable notify #1534

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 26, 2023
Merged

remove ssl-disable notify #1534

merged 1 commit into from
Jun 26, 2023

Conversation

rbelnap
Copy link
Contributor

@rbelnap rbelnap commented Feb 8, 2023

This notify shows as a 'change' in puppet reports, so if you have ssl disabled, every puppet run indicates a resource is changing - which is not the case. I also feel that it is not this puppet module's place to preach how to configure my systems - even if it is correct. #1512 (comment) suggests this as well and I agree.

@rbelnap rbelnap requested a review from a team as a code owner February 8, 2023 22:40
@CLAassistant
Copy link

CLAassistant commented Feb 8, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@puppet-community-rangefinder
Copy link

mysql::server is a class

Breaking changes to this file WILL impact these 79 modules (exact match):
Breaking changes to this file MAY impact these 49 modules (near match):

This module is declared in 140 of 580 indexed public Puppetfiles.

These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@rifter42
Copy link

rifter42 commented Apr 7, 2023

To add to what @rbelnap had already said, since this is indicated as a resource change, the notification restarts mysql on each run which is completely unacceptable. This has be fixed soon.

@chelnak
Copy link
Contributor

chelnak commented Apr 7, 2023

@rbelnap Can you sign the CLA please?

@rbelnap
Copy link
Contributor Author

rbelnap commented Apr 19, 2023

@chelnak the CLA is signed, is there anything preventing this from being merged?

@rbelnap
Copy link
Contributor Author

rbelnap commented Jun 23, 2023

Just checking in to see how I can help get this merged - I'd rather not keep maintaining a fork for such a small change. If there are any additional issues regarding the change, I'd welcome a discussion 😄

@chelnak
Copy link
Contributor

chelnak commented Jun 24, 2023

I see no issues with this being merged. I'll send a message to the team and hopefully it can get sorted on Monday.

@jordanbreen28
Copy link
Contributor

Hey @rbelnap, can you rebase this off main and remove the merge commit and I’d be happy to merge! 💯

@rbelnap rbelnap force-pushed the remove_ssl_notify branch from f259925 to bc0e3d7 Compare June 26, 2023 13:43
@rbelnap
Copy link
Contributor Author

rbelnap commented Jun 26, 2023

@jordanbreen28 this should now have the needful, let me know if it needs anything further.

@jordanbreen28 jordanbreen28 merged commit 3f8ece0 into puppetlabs:main Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jordanbreen28 jordanbreen28 jordanbreen28 approved these changes

@alexjfisher alexjfisher Awaiting requested review from alexjfisher alexjfisher is a code owner

@bastelfreak bastelfreak Awaiting requested review from bastelfreak bastelfreak is a code owner

Assignees
No one assigned
Labels
bugfix community
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rbelnap @CLAassistant @rifter42 @chelnak @jordanbreen28 @ia-content