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.

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

Add parameter to not create empty files when no fragments are defined #766

Merged
merged 2 commits into from Apr 11, 2023
Merged

Add parameter to not create empty files when no fragments are defined #766

merged 2 commits into from Apr 11, 2023

Conversation

JonasVerhofste
Copy link
Contributor

By default concat would always create an empty file if no fragment was defined. Sometimes we don't want a file unless it has content.

@JonasVerhofste JonasVerhofste requested a review from a team as a code owner March 29, 2023 16:03
@CLAassistant
Copy link

CLAassistant commented Mar 29, 2023 •
edited

CLA assistant check
All committers have signed the CLA.

@puppet-community-rangefinder
Copy link

concat_file is a type

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

concat is a type

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

This module is declared in 169 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.

@JonasVerhofste JonasVerhofste force-pushed the feature/no_empty_file branch 2 times, most recently from 165ddaf to 1e0308a Compare March 29, 2023 16:56
@JonasVerhofste
Add parameter to not create empty files when no fragments are defined
6f2c7d3 
By default concat would always create an empty file if no fragment was
defined. Sometimes we don't want a file unless it has content.
@JonasVerhofste
Update reference with rake strings:generate:reference
2b61a6f
@JonasVerhofste
Copy link
Contributor Author

Ran a rubocop:auto_correct, should be good now.

Also, this is a non-breaking change. Default behaviour is how it was before the param existed.

@JonasVerhofste
Copy link
Contributor Author

@LukasAud Seems like the two failing jobs are due to a provisioning error?

LukasAud
LukasAud approved these changes Apr 11, 2023
View reviewed changes
Copy link
Contributor

@LukasAud LukasAud left a comment

Choose a reason for hiding this comment

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

LGTM

@LukasAud
Copy link
Contributor

Waiting for acceptance to finish before merging.

@LukasAud LukasAud merged commit 0518492 into puppetlabs:main Apr 11, 2023
42 checks passed
@JonasVerhofste JonasVerhofste deleted the feature/no_empty_file branch April 11, 2023 14:08
@elfranne elfranne mentioned this pull request Apr 13, 2023
Closed
@elofu17
Copy link

elofu17 commented Apr 24, 2023 •
edited

Did this change introduce a regression?

I had puppetlabs/concat v7.3.0 and saz/ssh v10.0.0 installed on my puppet7 server.
Today I upgraded to concat v8.0.0 and ssh v10.1.0 and get the following error:

Error: Could not retrieve catalog from remote server: Error 500 on SERVER: Server Error: no parameter named 'create_empty_file' (file:  /etc/puppetlabs/code/environments/production/thirdparty_modules/concat/manifests/init.pp, line: 126) on Concat_file[/etc/ssh/sshd_config] (file: /etc/puppetlabs/code/environments/production/thirdparty_modules/concat/manifests/init.pp, line: 126) on node foobar
Warning: Not using cache on failed catalog
Error: Could not retrieve catalog; skipping run

I immediately reverted back to concat v7.3.0 (I let saz/ssh v10.1.0 remain) and everything started working fine again.

I see in the changelog for puppetlabs/concat v7.4.0, that changes were made for "create_empty_file" stuff, so this PR seems relevant. :)

Unfortunately I needed to stress my revert to get all my puppet nodes running, so I don't have any more debug information.

The code calling Concat_file[/etc/ssh/sshd_config] is Steffen Siegler's module https://forge.puppet.com/modules/saz/ssh/readme v10.1.0.

@JonasVerhofste
Copy link
Contributor Author

JonasVerhofste commented Apr 24, 2023 •
edited

@elofu17 This sounds very much alike #772 (see the mention above your comment).

The module you mention (saz/ssh) has the same "outdated" version requirement (i.e. lower than 8).

Could you test with puppetlabs/concat v7.4.0? That version should have the new param as well but should work, given the metadata of saz/ssh v10.1.0. If that works, then metadata is here the issue as well. :)

@elofu17
Copy link

elofu17 commented Apr 24, 2023

I will upgrade concat from 7.3.0 to 7.4.0 now and see.

@elofu17
Copy link

elofu17 commented Apr 24, 2023 •
edited

Identical error with 7.4.0 as with 8.0.0.
...so I'm reverting back to 7.3.0 again.

@siebrand
Copy link

Same issue here with puppet-nginx 4.4.0 and puppetlabs-concat 8.0.0, as well as with puppetlabs-concat 7.4.0.

Server Error: no parameter named 'create_empty_file' (file: /etc/puppetlabs/code/environments/production/modules/concat/manifests/init.pp, line: 126) on Concat_file[/etc/nginx/sites-available/00-http-php-80.conf] (file: /etc/puppetlabs/code/environments/production/modules/concat/manifests/init.pp, line: 126)

Highest version that works is concat 7.3.3.

@JonasVerhofste
Copy link
Contributor Author

@siebrand @elofu17 Just to confirm, you did run type generation (puppet generate types --environment ${env}) after bumping the module, right? Because this looks a lot like the manifest being updated, but puppet not correctly knowing about the new param in the type.

@elofu17
Copy link

elofu17 commented Apr 26, 2023 •
edited

Nope. :-) I don't even know what you are saying to be honest. :)

I've always just bumped the module version number in my Puppetfile.
On the next r10k run on the puppet-server, the environment(s) get the new 3rd party modules downloaded and populated.

Then a client node performs a puppet run, and the error is triggered.

I.e. I don't restart the puppet server, I don't clear its caches, I don't generate types or anything. Is this wrong?

@elofu17
Copy link

elofu17 commented Apr 26, 2023

I just tested upgrading to v8.0.0 again, and then restarted the puppet server.
Now it works :-)

It sounds like I was using some cached version instead of the new.
Interesting that this is the first time I notice any problems. :) Sure, some of the times when I bump the 3rd party modules I also upgrade the puppet server itself, and hence restart it == those times I wouldn't have this kind of problem. But I know that I have also bumped modules without a restart lots of times during the years, and this is the first time I have noticed a problem.

I'll remember to restart the server (or clear its cache) upon bumping modules in the future.

Sorry to have wasted your time, and thanks for your comments.

@JonasVerhofste
Copy link
Contributor Author

@elofu17 no worries, most important part is that it's fixed! A colleague pointed out that r10k probably takes care of the type generation, so I think you're good. (We're not using r10k, which is why we need to run the type generation manually)

@siebrand could you test bumping and then restarting puppetserver as well?

@ekohl
Copy link
Collaborator

ekohl commented Apr 26, 2023

FYI: https://www.puppet.com/docs/puppet/7/man/generate.html is the manpage for the command. It is important when using multiple environments because Puppetserver sometimes has a hard time isolating them. It's even the first item in the r10k FAQ: https://github.com/puppetlabs/r10k/blob/main/doc/faq.mkd#how-can-run-i-puppet-generate-types-for-each-changed-environment-during-deployment

@siebrand
Copy link

siebrand commented May 1, 2023

@JonasVerhofste I can confirm that the issue with 7.4.0/8.0.0 does not surface after a restart of puppetserver. Thank you for the help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LukasAud LukasAud LukasAud approved these changes

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@JonasVerhofste @CLAassistant @LukasAud @elofu17 @siebrand @ekohl