(CAT-1608) - PDK update #2501
(CAT-1608) - PDK update #2501
Conversation
Ramesh7
requested review from
bastelfreak,
ekohl,
smortex and
a team
as code owners
| default_facts.deep_merge!(YAML.safe_load(File.read(f), permitted_classes: [], permitted_symbols: [], aliases: true)) | ||
| default_facts.merge!(YAML.safe_load(File.read(f), permitted_classes: [], permitted_symbols: [], aliases: true)) |
There was a problem hiding this comment.
This may not be better. There are some pitfalls with (deep)merging listed here:
There was a problem hiding this comment.
hmm the rubocop was complaining, thanks for the heads-up.
There was a problem hiding this comment.
Was just going through the existing code deep_merge and the change merge, looks like deep_merge never got succeded as I can see warning in logs, but now introducing to merge it started failing some of specs with apache and mysql modules :
WARNING: Unable to load /home/runner/work/puppetlabs-apache/puppetlabs-apache/spec/default_facts.yml: undefined method `deep_merge!' for {:puppetversion=>"7.27.0", :facterversion=>"4.5.2"}:Hash
There was a problem hiding this comment.
I think maybe @ekohl can elaborate because he worked on the fix linked above and told me a few times that I should not use merge() and use the override_facts() function instead. This part of the patch has some info about the problem deep_merge pose:
There was a problem hiding this comment.
Anyways I think in this spec_helper we are trying to set some default facts which seems to be good, with override_facts make sense in actual spec tests where we already have implemented at multiple places.
There was a problem hiding this comment.
Dang, just saw in the context below that this is not altering the actual facts, but default_facts which is merged on line 36, so this seems okay.
Anyway, the module seems to not use any of these default facts. I wonder if it makes sense to keep them but I guess they serve as "example" for the PDK?
Anyway, LGTM!
Ramesh7
force-pushed
the
CAT-1608-pdk-update
branch
2 times, most recently
from
50439a4 to
5da0d4e
Compare
Ramesh7
force-pushed
the
CAT-1608-pdk-update
branch
from
5da0d4e to
791b599
Compare
| default_facts.deep_merge!(YAML.safe_load(File.read(f), permitted_classes: [], permitted_symbols: [], aliases: true)) | ||
| default_facts.merge!(YAML.safe_load(File.read(f), permitted_classes: [], permitted_symbols: [], aliases: true)) |
There was a problem hiding this comment.
Dang, just saw in the context below that this is not altering the actual facts, but default_facts which is merged on line 36, so this seems okay.
Anyway, the module seems to not use any of these default facts. I wonder if it makes sense to keep them but I guess they serve as "example" for the PDK?
Anyway, LGTM!
Summary
PDK update.
Checklist
puppet apply)