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 less restrictive env module merge #1107

Merged
merged 2 commits into from
Dec 5, 2020

Conversation

reidmv
Copy link
Contributor

@reidmv reidmv commented Dec 2, 2020

By default, allow a module defined in an environment to override a copy
defined in a Puppetfile, rather than hard-failing the deploy.

This allows for a more frictionless path to using environment modules.

This commit changes default behavior of an experimental feature.

@reidmv reidmv requested a review from a team December 2, 2020 01:17
@reidmv reidmv force-pushed the module_conflicts-env-setting branch 2 times, most recently from 8edf6a2 to bc955aa Compare December 2, 2020 03:15
By default, allow a module defined in an environment to override a copy
defined in a Puppetfile, rather than hard-failing the deploy.

This allows for a more frictionless path to using environment modules.
@reidmv reidmv force-pushed the module_conflicts-env-setting branch from bc955aa to 50acb3f Compare December 2, 2020 17:52
dhollinger
dhollinger previously approved these changes Dec 4, 2020
Copy link

@dhollinger dhollinger left a comment

Choose a reason for hiding this comment

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

Good work. I like it!

doc/dynamic-environments/configuration.mkd Outdated Show resolved Hide resolved
doc/dynamic-environments/configuration.mkd Outdated Show resolved Hide resolved
lib/r10k/environment/with_modules.rb Outdated Show resolved Hide resolved
lib/r10k/environment/with_modules.rb Outdated Show resolved Hide resolved
spec/unit/environment/with_modules_spec.rb Outdated Show resolved Hide resolved
spec/unit/environment/with_modules_spec.rb Show resolved Hide resolved
spec/unit/environment/with_modules_spec.rb Show resolved Hide resolved
CHANGELOG.mkd Outdated Show resolved Hide resolved
Co-authored-by: Molly Waggett <mwaggett@alumni.reed.edu>
@reidmv reidmv force-pushed the module_conflicts-env-setting branch from d6264cd to 8c097fe Compare December 5, 2020 00:07
@reidmv
Copy link
Contributor Author

reidmv commented Dec 5, 2020

@mwaggett Thanks so much for the detailed review! I think I've implemented all the suggestions. Ready for re-review.

Copy link
Contributor

@mwaggett mwaggett left a comment

Choose a reason for hiding this comment

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

Looks great! Just one quick question.

case conflict_opt = @options[:module_conflicts]
when 'override_puppetfile'
logger.debug log_msg
when 'override_puppetfile_and_warn', nil
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't notice this before, but what's the nil here for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the setting isn't specified, the option is nil. Default behavior. I don't think r10k has a nice way of using schemas or other systems for this kind of thing? Could be wrong. Any suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

ahhhh, gotcha. I think it's fine, I'm just a little out of practice at reading ruby haha 😅
I had skimmed and thought that the first case was the default, so wasn't sure what this was doing here in the middle, but I see now.

@mwaggett
Copy link
Contributor

mwaggett commented Dec 5, 2020

@dhollinger Would you mind taking another look, since there have been changes since your review?

Copy link

@dhollinger dhollinger left a comment

Choose a reason for hiding this comment

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

LGTM

@mwaggett mwaggett merged commit e6831cb into puppetlabs:master Dec 5, 2020
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.

3 participants