use correct reference when using strip_component #1240

b4ldr · 2021-11-03T15:22:19Z

The current implmnetation sets the environment name to the "striped"
version of the name, this is then later used as the git reference when
syncing environments.

With the following configuration

sources:
  b4ldr:
    strip_component: "wmcs/"
    ignore_branch_prefixes:
    - production
    - ops
    - sandbox
    - pontoon
    remote: https://github.com/b4ldr/puppet-1

where upstream has a branch with wmcs/production then we see the
following error

Unable to sync repo to unresolvable ref 'production' at
/etc/puppet/code/environments/production/.git

If upstream also has a production branch then the above configuration
ends up syncing /etc/puppet/code/environments/production/ with the
remote production branch. even thought it should be striped via
ignore_branch_prefixes

This PR updates the name object to keep hold of the original name and
store it as the ref

CLAassistant · 2021-11-03T15:22:31Z

All committers have signed the CLA.

Magisus · 2021-11-10T20:11:09Z

This fix makes sense. Could you add a test, maybe in here, showing that your new ref field contains the unmodified name (while name still gets the derived version)?

b4ldr · 2021-11-18T19:39:46Z

Sorry for the delay, have updated with additional rspec test now

Magisus · 2021-11-18T22:15:53Z

So I was testing this out, and I think you're missing a line in the sample config you give. It should also have strip_component: "wmcs/", right? If I add that and deploy your control repo, I see the issue you're seeing. Is that correct?

Magisus

Other than that small but important update to the commit message, I think this looks good! It definitely fixes the issue.

lib/r10k/environment/name.rb

The current implmnetation sets the environment name to the "striped" version of the name, this is then later used as the git reference when syncing environments. With the following configuration ```yaml sources: b4ldr: strip_component: "wmcs/" ignore_branch_prefixes: - production - ops - sandbox - pontoon remote: https://github.com/b4ldr/puppet-1 ``` where upstream has a branch with wmcs/production then we see the following error Unable to sync repo to unresolvable ref 'production' at /etc/puppet/code/environments/production/.git If upstream also has a production branch then the above configuration ends up syncing /etc/puppet/code/environments/production/ with the remote production branch. even thought it should be striped via `ignore_branch_prefixes` This PR updates the name object to keep hold of the original name and store it as the ref

b4ldr · 2021-11-19T18:08:48Z

So I was testing this out, and I think you're missing a line in the sample config you give. It should also have strip_component: "wmcs/", right? If I add that and deploy your control repo, I see the issue you're seeing. Is that correct?

yes, updated

mwaggett

Would you mind adding an entry to the 'Unreleased' section of the CHANGELOG describing this fix?

mwaggett · 2021-11-19T19:10:19Z

lib/r10k/environment/name.rb

+      attr_reader :name, :ref
+      attr_reader :name, :ref


This doesn't need to be here twice.

mwaggett · 2021-11-19T19:10:35Z

lib/r10k/environment/name.rb

      # @!attribute [r] name
-      #   @return [String] The unmodified name of the environment
-      attr_reader :name
+      #   @return [String] The name of the environment (with component striped)


What does 'component' refer to?

mwaggett · 2021-11-19T19:25:30Z

spec/unit/environment/name_spec.rb

@@ -149,6 +165,8 @@
        it "replaces invalid characters in #{branch} with underscores" do
          bn = described_class.new(branch.dup, {:correct => true})
          expect(bn.dirname).to eq branch.gsub(/\W/, '_')
+          expect(bn.name).to eq branch


huh, I would have thought this would also get corrected, but I guess I'm not totally sure where all the name is used.

Yeah. After taking a look through it, I realized that it's currently inconsistent. The original implementation of strip_component introduced the inconsistency. I'll take responsibility for that. There would ideally be a clear separation between the input value given to R10K::Environment::Name and the derived code-environment name on disk (currently called #dirname). Bit of work to clean it all up at this point.

ahh. Do you think it's a worthwhile bit of work to do eventually? If so, would you mind opening a ticket for it?

reidmv · 2021-11-19T19:38:10Z

As I'm thinking about this I have some thoughts on how to clarify what's going on here, without mixing in an assumption in R10K::Environment::Name that a VCS is involved (since not all environments come from version control systems, or have "references"). I will push some suggestions in just a bit.

The utility value is in being able to differentiate between the string used to create the R10K::Environment::Name object, and the #name attribute return value, which may have been transformed or modified from the original input. In order not to build in assumptions about where the original name came from (it isn't necessarily related to Git), use the general method `original_name` to differentiate from `name` (rather than the Git-specific term `ref`). This commit also changes some method names in R10K::Source::Git to clarify there the difference between a branch name and an environment name.

reidmv · 2021-11-19T20:09:44Z

@b4ldr I've suggested some generalizations in 6c95162. The biggest changes:

Renaming R10K::Environment::Name#ref to R10K::Environment::Name#original_name
Updating the documentation for the old and new R10K::Environment::Name attributes

The rest of the changes in the commit are just renaming some variables in R10K::Source::Git to better differentiate between branch names vs. environment names, since they can be different.

Unless you have any objections or feedback we'll keep the process moving forward. 🙂

reidmv

I have a commit in here myself now so somebody should sign off on that, but I'm 👍 overall

Magisus

Thanks for weighing in here, Reid, I agree, this is better.

Magisus · 2021-11-19T23:42:40Z

Dang, was really hoping git would be able to resolve that without a conflict :P

mwaggett · 2021-11-19T23:46:57Z

Let us know what you think of these changes, @b4ldr ! And if you wouldn't mind rebasing to resolve the merge conflicts in the CHANGELOG, that would be appreciated (we didn't want to force-push to your branch)!

b4ldr · 2021-11-20T00:04:02Z

Let us know what you think of these changes, @b4ldr ! And if you wouldn't mind rebasing to resolve the merge conflicts in the CHANGELOG, that would be appreciated (we didn't want to force-push to your branch)!

changes all good with me, think i have rebased and unblocked but let me know if not

Magisus · 2021-11-20T00:05:11Z

Looks good, we'll get this in!

b4ldr requested a review from a team November 3, 2021 15:22

mwaggett added awaiting user feedback bug labels Nov 11, 2021

b4ldr force-pushed the fix_strip_component branch from eb51c5e to 51c9856 Compare November 18, 2021 19:39

Magisus approved these changes Nov 18, 2021

View reviewed changes

reidmv reviewed Nov 18, 2021

View reviewed changes

lib/r10k/environment/name.rb Outdated Show resolved Hide resolved

b4ldr force-pushed the fix_strip_component branch from 51c9856 to 64c8c87 Compare November 19, 2021 18:11

mwaggett reviewed Nov 19, 2021

View reviewed changes

Fix discovered rspec errors

f52d0a4

reidmv approved these changes Nov 19, 2021

View reviewed changes

Magisus approved these changes Nov 19, 2021

View reviewed changes

(maint) Update Changelog

63995d5

mwaggett approved these changes Nov 19, 2021

View reviewed changes

b4ldr added 2 commits November 20, 2021 01:01

Merge branch 'main' into fix_strip_component

b2189b8

Update CHANGELOG.mkd

5ce3cd1

mwaggett approved these changes Nov 20, 2021

View reviewed changes

Magisus merged commit 120d7e3 into puppetlabs:main Nov 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use correct reference when using strip_component #1240

use correct reference when using strip_component #1240

b4ldr commented Nov 3, 2021 •

edited

Loading

CLAassistant commented Nov 3, 2021 •

edited

Loading

Magisus commented Nov 10, 2021

b4ldr commented Nov 18, 2021

Magisus commented Nov 18, 2021

Magisus left a comment

b4ldr commented Nov 19, 2021

mwaggett left a comment

mwaggett Nov 19, 2021

mwaggett Nov 19, 2021

mwaggett Nov 19, 2021

reidmv Nov 19, 2021

mwaggett Nov 19, 2021

reidmv commented Nov 19, 2021

reidmv commented Nov 19, 2021

reidmv left a comment

Magisus left a comment

Magisus commented Nov 19, 2021

mwaggett commented Nov 19, 2021

b4ldr commented Nov 20, 2021

Magisus commented Nov 20, 2021

use correct reference when using strip_component #1240

use correct reference when using strip_component #1240

Conversation

b4ldr commented Nov 3, 2021 • edited Loading

CLAassistant commented Nov 3, 2021 • edited Loading

Magisus commented Nov 10, 2021

b4ldr commented Nov 18, 2021

Magisus commented Nov 18, 2021

Magisus left a comment

Choose a reason for hiding this comment

b4ldr commented Nov 19, 2021

mwaggett left a comment

Choose a reason for hiding this comment

mwaggett Nov 19, 2021

Choose a reason for hiding this comment

mwaggett Nov 19, 2021

Choose a reason for hiding this comment

mwaggett Nov 19, 2021

Choose a reason for hiding this comment

reidmv Nov 19, 2021

Choose a reason for hiding this comment

mwaggett Nov 19, 2021

Choose a reason for hiding this comment

reidmv commented Nov 19, 2021

reidmv commented Nov 19, 2021

reidmv left a comment

Choose a reason for hiding this comment

Magisus left a comment

Choose a reason for hiding this comment

Magisus commented Nov 19, 2021

mwaggett commented Nov 19, 2021

b4ldr commented Nov 20, 2021

Magisus commented Nov 20, 2021

b4ldr commented Nov 3, 2021 •

edited

Loading

CLAassistant commented Nov 3, 2021 •

edited

Loading