(RK-308) Allow respecting `forge` declaration in puppetfile #1214

Magisus · 2021-09-07T21:16:12Z

This commit adds a new setting forge.allow_puppetfile_override, that
when true will cause a forge declaration in a Puppetfile to override
the forge.baseurl setting for contacting the Forge. This allows users
to configure per-environment Forge URLs, for example, if an internal
Forge is used for a test environment and the live Forge for production.
Previously, a user would have had to choose one or the other. The
setting is false by default, since many users have erroneous forge
declarations in their Puppetfiles that were previously completely
ignored.

Magisus · 2021-09-07T21:19:12Z

So if I understand right, we shouldn't need to do anything to the set_forge function in the old Puppetfile class, because it will just delegate to the one in ModuleLoader::Puppetfile.

Magisus · 2021-09-07T21:49:32Z

lib/r10k/module_loader/puppetfile.rb

+          # This method will append a trailing slash to the string, in place, so we
+          # dup it to avoid different behavior in the value of our instance var
+          # when `allow_puppetfile_forge` is true vs false
+          PuppetForge.host = forge.dup


The dup is probably unnecessary, seeing as we don't really use this instance var for anything that I can see. Lemme know what you think. Also, if we're not using @forge... do we need the instance variable at all?

yeah, this is confusing to me.. I would be in favor of removing @forge and the various set_forge methods if they're not actually doing anything, since it sorta seems like they're not? but it's unclear. Or perhaps replace the set_forge methods with ones that actually do something.. I would have imagined something calling set_forge(forge_from_puppetfile) and then having PuppetForge.host = @forge somewhere where it's actually being used? But I don't really know, everything is a mess haha.

As the comment at the top of this block says, set_forge is part of the Puppetfile parsing DSL. It is called when the Puppetfile is evaluated by that DSL. Until now, it has set this instance variable, but been a noop since, so far as I could tell, the var isn't used (we were deliberately ignoring forge declarations in the Puppetfile).

The actual way to make the forge declaration be used is to set the host field for the PuppetForge gem. That's what we do when reading it from settings. I didn't delete the instance variable, in case something else is expecting it be set, but I think maybe that's not the case. This seems even more likely given there's no getter for it. I can try that and see what happens.

yeah, I know the DSL has a method that calls this, but I didn't see anywhere calling the DSL's forge method, so wondering if that could be removed too.

The DSL represents an implementation of how to evalute a Puppetfile as a Ruby script. So if a Puppetfile has

forge "my.awesome.forge mod "puppetlabs/apache"

forge and mod there are Ruby function calls, where the definition is what's in the DSL class. So when instance_eval happens, we evaluate the Ruby script that is the Puppetfile in the context of the DSL class, which has those methods defined, effectively calling forge, then mod.

I was trying to look for a good example of using instance_eval this way, but it seems like most people use it to add methods to individual objects, instead of actually executing code directly like we're doing here. The docs might help though: https://www.rubydoc.info/stdlib/core/BasicObject:instance_eval

oh right, ugh. didn't realize just greping wouldn't be enough 😅 but that makes sense.

Magisus · 2021-09-07T21:50:45Z

Manually tested this, seems to work.

Magisus · 2021-09-07T22:01:53Z

Note this does not support custom auth for the overridden Forge. It will use the same auth as the normal baseurl, if any is configured. Fixing that is a separate effort.

justinstoller · 2021-09-07T23:33:05Z

Looks good, but I think docs need updating for the setting.

cdenneen · 2021-09-07T23:43:13Z

Note this does not support custom auth for the overridden Forge. It will use the same auth as the normal baseurl, if any is configured. Fixing that is a separate effort.

So this won’t work with the Bearer token fix? Is that work being tracked anywhere?

For some reason I thought here it would work if done as Basic Auth but that might have been just —module_repository parameter for .fixtures.yml

Magisus · 2021-09-07T23:55:32Z

If you configure the Bearer token in the forge section of r10k.yaml, that token will be used with all forges declared in Puppetfiles. That might work for some use cases, but likely not for all of them. We should file a ticket about supporting per-environment credentials if you know that will be needed. I'm not sure what that should look like from a security/UX perspective...

I'm not super familiar with the Basic Auth usecase... I think I saw some ticket where someone thought it didn't work even now, with regular baseurl, but I'm not sure.

cdenneen · 2021-09-08T00:48:25Z

If you configure the Bearer token in the forge section of r10k.yaml, that token will be used with all forges declared in Puppetfiles. That might work for some use cases, but likely not for all of them. We should file a ticket about supporting per-environment credentials if you know that will be needed. I'm not sure what that should look like from a security/UX perspective...

I'm not super familiar with the Basic Auth usecase... I think I saw some ticket where someone thought it didn't work even now, with regular baseurl, but I'm not sure.

Basic Auth would be https://user:pass@forge/api

Magisus · 2021-09-08T15:20:27Z

If you already know that basic auth does work (I have not been able to try it, but might be able to set something up), and are comfortable putting a password in your Puppetfile, then I think that could be a workaround. I sort of ruled it out because Puppetfiles are generally committed to source control, and putting a password in them seems like a bad idea. But I guess that would depend a lot on your individual context.

CHANGELOG.mkd

doc/puppetfile.mkd

mwaggett · 2021-09-09T17:41:38Z

lib/r10k/module_loader/puppetfile.rb

+          # This method will append a trailing slash to the string, in place, so we
+          # dup it to avoid different behavior in the value of our instance var
+          # when `allow_puppetfile_forge` is true vs false
+          PuppetForge.host = forge.dup


yeah, this is confusing to me.. I would be in favor of removing @forge and the various set_forge methods if they're not actually doing anything, since it sorta seems like they're not? but it's unclear. Or perhaps replace the set_forge methods with ones that actually do something.. I would have imagined something calling set_forge(forge_from_puppetfile) and then having PuppetForge.host = @forge somewhere where it's actually being used? But I don't really know, everything is a mess haha.

spec/unit/module_loader/puppetfile_spec.rb

This commit adds a new setting `forge.allow_puppetfile_override`, that when true will cause a `forge` declaration in a Puppetfile to override the `forge.baseurl` setting for contacting the Forge. This allows users to configure per-environment Forge URLs, for example, if an internal Forge is used for a test environment and the live Forge for production. Previously, a user would have had to choose one or the other. The setting is false by default, since many users have erroneous `forge` declarations in their Puppetfiles that were previously completely ignored.

Magisus · 2021-09-09T18:49:23Z

Updated to remove the @forge instance variable. Let me know what you think.

CHANGELOG.mkd

mwaggett · 2021-09-09T20:49:12Z

spec/unit/module_loader/puppetfile_spec.rb

+      it 'is ignored if `allow_puppetfile_override` is false' do
+        @path = File.join(PROJECT_ROOT, 'spec', 'fixtures', 'unit', 'puppetfile', 'forge-override')
+        puppetfile = R10K::ModuleLoader::Puppetfile.new(basedir: @path, overrides: { forge: { allow_puppetfile_override: false } })
+        expect(PuppetForge).not_to receive(:host=).with("my.custom.forge.com")


Is this necessary if we're testing the value of PuppetForge.host below?

What I like about this one is it ties us back to the specific value from the Puppetfile that we want to be ignored.

mwaggett · 2021-09-09T20:51:49Z

lib/r10k/module_loader/puppetfile.rb

@@ -113,7 +111,10 @@ def add_module_metadata(name, info)

      # @param [String] forge
      def set_forge(forge)
-        @forge = forge
+        if @allow_puppetfile_forge
+          logger.debug _("Using Forge from Puppetfile: %{forge}") % { forge: forge }


Should we also log that we're ignoring the forge setting from the Puppetfile if @allow_puppetfile_forge isn't true?

I could go either way, it's default behavior and has been that way a really long time. But I can add it.

Do we think we'd want to make this the default behavior in the next major release? If so, I think it could be good to log a message about it. ¯_(ツ)_/¯
Otherwise, if we think this will be opt-in forever, I'm good with leaving it out.

I seriously doubt we will ever make this the default behavior. We're leaning away from having people put more things in their puppetfiles.

In that case, I'm good with leaving out the logging. (sorry for the back and forth 😅 )

Having tested this out, I think I'll leave it in, actually. There's isn't any other logging currently at debug or debug1 that tells you which forge you're using.

This class does not make any use of its `forge` instance variable, nor does it expose it via a getter. Since it is unused, this commit removes it and the associated tests. Note that the old `Puppetfile` class still exposes a `forge` instance variable that is hard-coded to a default, and does not respect either the `forge.baseurl` setting or a `forge` declaration in a Puppetfile. This var is also unused within r10k (`PuppetForge.host` is set directly elsewhere), but might be used by people using the `Puppetfile` class by itself.

Magisus requested a review from a team September 7, 2021 21:16

Magisus force-pushed the forge-puppetfile branch from 693c33f to 53b1a05 Compare September 7, 2021 21:17

Magisus force-pushed the forge-puppetfile branch 2 times, most recently from 2c84f95 to be41321 Compare September 7, 2021 21:48

Magisus commented Sep 7, 2021

View reviewed changes

Magisus mentioned this pull request Sep 7, 2021

(RK-308) Use alternate forge from Puppetfile #1134

Closed

Magisus force-pushed the forge-puppetfile branch from be41321 to 068db78 Compare September 7, 2021 22:06

Magisus changed the title ~~(RK-308) Allow respecing forge declaration in puppetfile~~ (RK-308) Allow respecting forge declaration in puppetfile Sep 7, 2021

Magisus force-pushed the forge-puppetfile branch from 068db78 to 98b6cb1 Compare September 7, 2021 22:10

Magisus force-pushed the forge-puppetfile branch from 98b6cb1 to 831fd94 Compare September 7, 2021 23:52

mwaggett added feature has jira ticket Label for issues that are duplicated in Jira labels Sep 9, 2021

mwaggett reviewed Sep 9, 2021

View reviewed changes

Magisus force-pushed the forge-puppetfile branch from 831fd94 to fec2610 Compare September 9, 2021 18:47

mwaggett reviewed Sep 9, 2021

View reviewed changes

Magisus force-pushed the forge-puppetfile branch from fec2610 to b149251 Compare September 9, 2021 21:49

mwaggett approved these changes Sep 9, 2021

View reviewed changes

mwaggett merged commit 830aedd into puppetlabs:main Sep 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(RK-308) Allow respecting `forge` declaration in puppetfile #1214

(RK-308) Allow respecting `forge` declaration in puppetfile #1214

Magisus commented Sep 7, 2021 •

edited

Loading

Magisus commented Sep 7, 2021

Magisus Sep 7, 2021 •

edited

Loading

mwaggett Sep 9, 2021

Magisus Sep 9, 2021 •

edited

Loading

mwaggett Sep 9, 2021

Magisus Sep 9, 2021

mwaggett Sep 9, 2021

Magisus commented Sep 7, 2021

Magisus commented Sep 7, 2021

justinstoller commented Sep 7, 2021

cdenneen commented Sep 7, 2021

Magisus commented Sep 7, 2021 •

edited

Loading

cdenneen commented Sep 8, 2021

Magisus commented Sep 8, 2021

mwaggett Sep 9, 2021

Magisus commented Sep 9, 2021

mwaggett Sep 9, 2021

Magisus Sep 9, 2021

mwaggett Sep 9, 2021

Magisus Sep 9, 2021

mwaggett Sep 9, 2021

Magisus Sep 9, 2021

mwaggett Sep 9, 2021

Magisus Sep 9, 2021

(RK-308) Allow respecting forge declaration in puppetfile #1214

(RK-308) Allow respecting forge declaration in puppetfile #1214

Conversation

Magisus commented Sep 7, 2021 • edited Loading

Magisus commented Sep 7, 2021

Magisus Sep 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Magisus Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Magisus commented Sep 7, 2021

Magisus commented Sep 7, 2021

justinstoller commented Sep 7, 2021

cdenneen commented Sep 7, 2021

Magisus commented Sep 7, 2021 • edited Loading

cdenneen commented Sep 8, 2021

Magisus commented Sep 8, 2021

Choose a reason for hiding this comment

Magisus commented Sep 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

(RK-308) Allow respecting `forge` declaration in puppetfile #1214

(RK-308) Allow respecting `forge` declaration in puppetfile #1214

Magisus commented Sep 7, 2021 •

edited

Loading

Magisus Sep 7, 2021 •

edited

Loading

Magisus Sep 9, 2021 •

edited

Loading

Magisus commented Sep 7, 2021 •

edited

Loading