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 strip_component source feature #1128

Merged
merged 2 commits into from
Mar 9, 2021

Conversation

reidmv
Copy link
Contributor

@reidmv reidmv commented Feb 24, 2021

This feature allows r10k to be configured to strip a leading component string from a source's environment names. This is useful because it allows e.g. Git branches to include organizational prefixes such as "environment/production", "environment/development", but r10k/Puppet to deploy and use environments named "production", "development" from these branches.

@reidmv reidmv force-pushed the strip_component branch 12 times, most recently from f125fc5 to 5f653f8 Compare February 24, 2021 20:06
@reidmv reidmv marked this pull request as ready for review February 24, 2021 20:07
@reidmv reidmv requested a review from a team February 24, 2021 20:07
This feature allows r10k to be configured to strip a leading component
string from a source's environment names. This is useful because it
allows e.g. Git branches to include organizational prefixes such as
"environment/production", "environment/development", but r10k/Puppet to
deploy and use environments named "production", "development" from these
branches.
@mwaggett
Copy link
Contributor

@reidmv What prompted this? My main concern is mostly just around adding yet more configuration for a project that already isn't well-understood and isn't getting a ton of love.

@reidmv
Copy link
Contributor Author

reidmv commented Feb 25, 2021

@mwaggett This was prompted by an internal Slack conversation relating to defining best practices setting customers up for success in the context of CD4PE.

Gist of it: trying to define a best practice for Git branch naming in the context of CD4PE, needing to find a way to balance

  1. CD4PE’s today-imposed requirement that deployment state is exposed in Git branches
  2. Our desire to give normalized Git workflow for users, with deployment state info intuitively corralled and user-ignorable
  3. The literal Puppet environment names also being as intuitive as possible, since they are exposed to users in a number of GUIs and APIs

However, that conversation is ongoing and needs input from the CD4PE product owner because r10k supporting this doesn't actually mean CD4PE supports it. So, totally fine to put this PR "on hold" for awhile while we figure all of that out.

@mwaggett
Copy link
Contributor

okay cool, thanks for the context! After reading that thread, I would agree that we need product input before deciding if this should go in. cc @ccaum @npwalker

@ccaum
Copy link

ccaum commented Feb 25, 2021

Thanks, Reid. My initial take is that this is very useful for separating the CD for PE managed branches in git vs the human readable Puppet environments and I'd like to see it implemented. However, it's not a great user setup experience and only something I'd expect professional services to setup for a user. Hopefully in the future we can remove the need for this setting altogether.

@reidmv
Copy link
Contributor Author

reidmv commented Feb 25, 2021

@ccaum totally agree on wanting to make it unnecessary—in r10k land, the exec source type is more oriented towards that "big" step forward, while this feature if implemented would be enabling more of a small, iterative polish kind of thing.

Notably, while the motivation for this came from CD4PE, it seems like it could improve OSP workflows too, and may be desirable for that reason even if CD4PE takes the big step forward and doesn't ever need something like this.

Thoughts on this as a stand-alone r10k OSP-oriented capability?

@@ -71,8 +71,24 @@ def dirname

private

def derive_prefix(source,prefix)
def derive_name(name, strip_component)
if strip_component == nil
Copy link
Member

Choose a reason for hiding this comment

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

Is false a value you could see users put in here? Should we instead handle "falsey" values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that would make sense. Updated to handle "falsey" instead of nil.

else
name
end
rescue TypeError, NoMethodError
Copy link
Member

Choose a reason for hiding this comment

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

IMO, it's simpler to test the class of the input then doing a rescue afterwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Funny how code structure sometimes reveals developer (my) chronological thought process, and highlights afterthoughts 😅 . Pushing improvement.

Copy link
Contributor

@npwalker npwalker left a comment

Choose a reason for hiding this comment

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

+1 Carl likes it so I'm good with it

@justinstoller justinstoller merged commit 0f571cc into puppetlabs:master Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants