(RK-390) Remove hardcoded default for git ref #1275

tvpartytonight · 2022-02-09T17:40:16Z

Previously r10k fell back to the 'main' ref if no other ref was
specified. With this patch, r10k will no longer fall back to that
ref, and error when no ref is found.

To mitigate breakage relying on the hardcoded ref, a user can
set default_ref in the r10k config to a ref of their
choice, mimicking the old behavior.

Please add all notable changes to the "Unreleased" section of the CHANGELOG in the format:

- (JIRA ticket) Summary of changes. [Issue or PR #](link to issue or PR)

mwaggett

This needs a changelog update.

doc/dynamic-environments/configuration.mkd

lib/r10k/module/git.rb

lib/r10k/settings.rb

spec/unit/module/git_spec.rb

Magisus

This seems alright to me. @reidmv wanna check with you, since this came from your request.

reidmv

I don't remember the context for this, so I'm reviewing at face value here.

I have some concerns about the UX as given, but I think I understand the objective and I think it should be easy to simplify, unless perhaps there's something I've misunderstood about how this is supposed to work.

Here's my UX concern.

Today, a user might specify a module as follows:

- name:   puppetlabs/stdlib
  type:   git
  source: https://github.com/puppetlabs/puppetlabs-stdlib.git

We haven't specified a ref or version at the module level. We want to be able to omit that information, and specify a global default to use. So far so good. It makes sense.

So, in r10k.yaml, what should that setting be called?

I don't really like deploy.default_module_ref. I think either we're configuring module defaults, or we're configuring git defaults, so this setting belongs either under module.* somewhere, or git.* somewhere. I would lean towards making this a git default. So, I think we want the setting to be placed there in r10k.yaml:

# r10k.yaml
---
git:
  default_ref: main

Some uses of git will require a ref, but if the utilization permits falling back to a default, this is a good global place to define what that default should be.

If we put the default setting there, I'm back to feeling comfortably like this all more or less makes sense and is reasonably simple.

Next, this PR suggests adding an additional possible per-module setting of default_module_ref. E.g., a user could legally write:

- name:   puppetlabs/stdlib
  type:   git
  source: https://github.com/puppetlabs/puppetlabs-stdlib.git
  default_module_ref: main

This seems bad to me. It's confusing, and I'm not sure what the difference here is between this and just writing

- name:   puppetlabs/stdlib
  type:   git
  source: https://github.com/puppetlabs/puppetlabs-stdlib.git
  ref:    main

So, I would prefer that default_module_ref not be a legal option to pass to an individual module.

I would request two changes:

Change the setting from deploy.default_module_ref to git.default_ref.
Remove default_module_ref as an exposed per-module setting.

Would those adjustments to the user-facing part of this experience still cover the intended use case?

jarretlavallee · 2022-02-10T15:03:49Z

In support of what @reidmv said above, the changes currently causes the following warning for each forge module. This seems like it would be a git-only option.

WARN	 -> R10K::Module::Forge cannot handle option 'default_module_ref'
WARN	 -> R10K::Module::Forge cannot handle option 'default_module_ref'
WARN	 -> R10K::Module::Forge cannot handle option 'default_module_ref'

The error that comes from missing the default ref could be confusing for some users that did not read the changelog. Adding some context to the message can help users identify and correct the issue. Can we update it to say that there is no default branch in 4.0.0+?

ERROR	 -> Error during concurrent deploy of a module: Unable to manage Puppetfile content 'pe_status_check': Could not determine desired ref and no default provided
ERROR	 -> Unable to manage Puppetfile content 'pe_status_check': Could not determine desired ref and no default provided

This change is only for deploy, so r10k puppetfile install fails to use the new configuration option.

Another consideration is that we are changing how users should write their Puppetfile. The r10k puppetfile check validation should call out entries that are missing this. It could be a warning since the synax would be acceptable with the default branch option in the r10k configuration.

Magisus · 2022-02-10T15:45:25Z

I missed the bit about this new thing being available as a per-module setting, agreed we definitely don't want that: that's what ref etc. are for.

tvpartytonight · 2022-02-10T17:25:35Z

Next, this PR suggests adding an additional possible per-module setting of default_module_ref. E.g., a user could legally write:

This ^^ is incorrect; the puppetfile doc states(emphasis my own):

A default_module_ref can be specified in the r10k config

There's no mention of default_module_ref being an option in a Puppetfile, only that the r10k config now has a setting that the Puppetfile can override. It seems apparent that needs to be made more explicit.

I do think that @reidmv is correct that this should move over to the git namespace.

@jarretlavallee I will fix those incorrect warnings and add it to the puppetfile face.

reidmv · 2022-02-10T17:31:59Z

Next, this PR suggests adding an additional possible per-module setting of default_module_ref. E.g., a user could legally write:

This ^^ is incorrect; the puppetfile doc states(emphasis my own):

A default_module_ref can be specified in the r10k config

There's no mention of default_module_ref being an option in a Puppetfile, only that the r10k config now has a setting that the Puppetfile can override. It seems apparent that needs to be made more explicit.

Yeah, I was mixing looking at the docs (what should a user do) and looking at the code (what can a user legally do). If I'm reading the code correctly, the fact that we add :default_module_ref as a legal opt here means the user can write this in a Puppetfile, and it will be plumbed all the way through. I think that's how these opts work.

We already don't intend for people to use this opt themselves per-module; my comment is that we should ensure it isn't possible, even accidentally.

Previously r10k fell back to the 'main' ref if no other ref was specified. With this patch, r10k will no longer fall back to that ref, and error when no ref is found. To mitigate breakage relying on the hardcoded ref, a user can set `default_ref` in the r10k config to a ref of their choice, mimicking the old behavior.

mwaggett · 2022-02-10T21:30:30Z

doc/dynamic-environments/configuration.mkd

+set in the r10k config that will become the ref a module uses if not otherwise specified. This
+is the lowest priority setting for a module's `ref`. Read [here](../puppetfile.mkd#git) for more info.


Suggested change

set in the r10k config that will become the ref a module uses if not otherwise specified. This

is the lowest priority setting for a module's `ref`. Read [here](../puppetfile.mkd#git) for more info.

set in the r10k config, which will be used if a module's `ref` is not otherwise specified. This

is the lowest priority setting for a module's `ref`. See the [Puppetfile documentation](../puppetfile.mkd#git) for the recommended way to specify module `ref`s.

doc/puppetfile.mkd

lib/r10k/module/git.rb

lib/r10k/settings.rb

spec/unit/action/deploy/module_spec.rb

spec/unit/action/puppetfile/install_spec.rb

tvpartytonight · 2022-02-11T00:54:57Z

@reidmv and @jarretlavallee I believe I have addressed all your comments, so another review would great, please and thankyou.

One thing I didn't implement was adding puppetfile check to see if the ref is resolving. I experimented a bit with calling version in the initialize for the git class and converting errors to warnings, but that seemed to add a lot of generally unecessary lookups in the repo, and I am wary of slowing down puppetfile install or deploy environment/module unnecessarily. If you really want it I can look into it some more, but it's a non-trivial add for the syntax check to resolve the ref.

edit: Oh, actually it's probably really easy to just iterate through the modules during check and just run version on the git ones, which won't affect other r10k verbs. I'll do that next.

jarretlavallee · 2022-02-11T15:00:40Z

👍 The changes address the majority of my concerns and I have validated the functionality. The r10k puppetfile check is a nice to have and I do not consider it a requirement for this PR. The error message is clear enough for someone to diagnose this during deployment.

tvpartytonight · 2022-02-11T17:19:04Z

@jarretlavallee I put in the logic for the check and it is simpler and safe than running validate_ref, so all your concerns are addressed!

mwaggett · 2022-02-11T19:14:48Z

doc/puppetfile.mkd

@@ -135,6 +135,12 @@ operations when updating the repo, which can speed up install times. When
 Module versions can also be specified using `:branch` to track a specific
 branch reference.

+In r10k 3.x the default branch was hardcoded to `master`; in 4.x that was
+removed. A `default_ref` can be specified in the r10k config to
+to mimick that old behavior, but it is recommended to set the ref on a


Suggested change

to mimick that old behavior, but it is recommended to set the ref on a

to mimic that old behavior, but it is recommended to set the ref on a

mwaggett · 2022-02-11T19:46:01Z

spec/unit/action/puppetfile/check_spec.rb

@@ -15,11 +22,39 @@ def checker(opts = {}, argv = [], settings = {})
      to receive(:new).
      with({
        basedir: "/some/nonexistent/path",
+        overrides: {modules: {default_ref: nil}}


Why do we need to set this explicitly?

The check code will read it in everytime, so if it is not here, it will error with a mismatch in the argurments passed to new.

mwaggett · 2022-02-11T19:51:33Z

lib/r10k/module/git.rb

+  def validate_ref_defined
+    if @desired_ref.nil? && @default_ref.nil? && @default_override_ref.nil?
+      msg = "No ref defined for module #{@name}. Add a ref to the module definition "
+      msg << "or add a default in the r10k config for git:default_ref."


Suggested change

msg << "or add a default in the r10k config for git:default_ref."

msg << "or set git:default_ref in the r10k.yaml config to configure a global default ref."

mwaggett · 2022-02-11T22:50:04Z

lib/r10k/module/git.rb

@@ -147,7 +154,9 @@ def validate_ref(desired, default, default_override)
        msg << "or resolve default ref '%{default}'"
        vars[:default] = default
      else
-        msg << "and no default provided"
+        msg << "and no default provided. r10k no longer hardcodes 'master' as the default ref."
+        msg << "Consider setting this per module in the Puppetfile or setting git:default_ref"


Suggested change

msg << "Consider setting this per module in the Puppetfile or setting git:default_ref"

msg << "Consider setting a ref per module in the Puppetfile or setting git:default_ref"

tvpartytonight requested a review from a team February 9, 2022 17:40

mwaggett reviewed Feb 9, 2022

View reviewed changes

tvpartytonight force-pushed the RK-390 branch 2 times, most recently from 4dab311 to cc1b428 Compare February 9, 2022 23:24

Magisus approved these changes Feb 10, 2022

View reviewed changes

reidmv suggested changes Feb 10, 2022

View reviewed changes

tvpartytonight force-pushed the RK-390 branch from cc1b428 to c7de4e9 Compare February 10, 2022 20:28

tvpartytonight force-pushed the RK-390 branch from c7de4e9 to 921acf4 Compare February 10, 2022 20:49

mwaggett reviewed Feb 10, 2022

View reviewed changes

(SQUASH ME) PR Feedback

3285855

(SQUASH ME) Add ref check for puppetfile check

9fe76ee

mwaggett reviewed Feb 11, 2022

View reviewed changes

(SQUASH ME) PR feedback

45b1310

Magisus merged commit 97158a5 into puppetlabs:4.x Feb 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(RK-390) Remove hardcoded default for git ref #1275

(RK-390) Remove hardcoded default for git ref #1275

tvpartytonight commented Feb 9, 2022 •

edited

Loading

mwaggett left a comment

Magisus left a comment

reidmv left a comment

jarretlavallee commented Feb 10, 2022 •

edited

Loading

Magisus commented Feb 10, 2022

tvpartytonight commented Feb 10, 2022

reidmv commented Feb 10, 2022

mwaggett Feb 10, 2022

tvpartytonight commented Feb 11, 2022 •

edited

Loading

jarretlavallee commented Feb 11, 2022

tvpartytonight commented Feb 11, 2022

mwaggett Feb 11, 2022

mwaggett Feb 11, 2022

tvpartytonight Feb 11, 2022

mwaggett Feb 11, 2022

mwaggett Feb 11, 2022

mwaggett Feb 11, 2022

		set in the r10k config that will become the ref a module uses if not otherwise specified. This
		is the lowest priority setting for a module's `ref`. Read [here](../puppetfile.mkd#git) for more info.

	to mimick that old behavior, but it is recommended to set the ref on a
	to mimic that old behavior, but it is recommended to set the ref on a

	msg << "or add a default in the r10k config for git:default_ref."
	msg << "or set git:default_ref in the r10k.yaml config to configure a global default ref."

	msg << "Consider setting this per module in the Puppetfile or setting git:default_ref"
	msg << "Consider setting a ref per module in the Puppetfile or setting git:default_ref"

(RK-390) Remove hardcoded default for git ref #1275

(RK-390) Remove hardcoded default for git ref #1275

Conversation

tvpartytonight commented Feb 9, 2022 • edited Loading

mwaggett left a comment

Choose a reason for hiding this comment

Magisus left a comment

Choose a reason for hiding this comment

reidmv left a comment

Choose a reason for hiding this comment

jarretlavallee commented Feb 10, 2022 • edited Loading

Magisus commented Feb 10, 2022

tvpartytonight commented Feb 10, 2022

reidmv commented Feb 10, 2022

mwaggett Feb 10, 2022

Choose a reason for hiding this comment

tvpartytonight commented Feb 11, 2022 • edited Loading

jarretlavallee commented Feb 11, 2022

tvpartytonight commented Feb 11, 2022

mwaggett Feb 11, 2022

Choose a reason for hiding this comment

mwaggett Feb 11, 2022

Choose a reason for hiding this comment

tvpartytonight Feb 11, 2022

Choose a reason for hiding this comment

mwaggett Feb 11, 2022

Choose a reason for hiding this comment

mwaggett Feb 11, 2022

Choose a reason for hiding this comment

mwaggett Feb 11, 2022

Choose a reason for hiding this comment

tvpartytonight commented Feb 9, 2022 •

edited

Loading

jarretlavallee commented Feb 10, 2022 •

edited

Loading

tvpartytonight commented Feb 11, 2022 •

edited

Loading