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

Add exec environment source #1042

Merged
merged 3 commits into from
Apr 28, 2020
Merged

Conversation

reidmv
Copy link
Contributor

@reidmv reidmv commented Apr 3, 2020

Add a new environment source type, exec, to enable simple implementation of external environment sources.

The exec environment source runs an external command which is expected to return on stdout content compatible with the YAML environment source data format. The command may return the data in JSON or YAML form. The exec environment source is similar in purpose to Puppet's exec node terminus, used to implement external node classifiers (ENCs). R10k's exec source type allows the the implementation of external environment sources.

Video demo:
https://youtu.be/npfqRjaIxGo

The video goes for 20 minutes and includes how CD4PE/PE can use R10k with this feature. The R10k-specific part of the demo is presented early on.

@reidmv reidmv requested a review from a team April 3, 2020 19:31
lib/r10k/source/exec.rb Outdated Show resolved Hide resolved
@mwaggett
Copy link
Contributor

This looks good to me! Just wondering if there should be accompanying tests.

Also, someone more familiar with this codebase should look at this too, since I'm still new to this one. 😅

Copy link
Contributor

@adrienthebo adrienthebo left a comment

Choose a reason for hiding this comment

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

Looks good overall, thanks @reidmv! If possible let's use the r10k subprocess utility for command execution, simplify format handling if possible, and I'd like to see some basic test coverage for this. Once those concerns are concerned we should be good to merge this.

Expanding on the format handling, I'm mildly opposed to supporting multiple formats without a concrete use case; this is an easy simplification and avoids the mild heuristics that multiple formats entail. For comparison the puppet exec node terminus only supports YAML so we have prior art for supporting a single format. That being said, I don't feel strongly - if there's a compelling case for multiple formats, then we can proceed as-is.

Summary of the above:

  • Command execution with Subprocess
  • Format handling simplification (or documentation for multiple format use cases)
  • Test coverage

Ping me in the Puppet community Slack if you'd like to discuss this more. Thanks!

lib/r10k/source/exec.rb Show resolved Hide resolved
lib/r10k/source/exec.rb Outdated Show resolved Hide resolved
@reidmv reidmv force-pushed the exec-source branch 3 times, most recently from d6f8df2 to 5153dd9 Compare April 20, 2020 23:21
lib/r10k/source/exec.rb Outdated Show resolved Hide resolved
@adrienthebo adrienthebo dismissed their stale review April 21, 2020 19:07

Dismissing stale review

adrienthebo
adrienthebo previously approved these changes Apr 21, 2020
Copy link
Contributor

@adrienthebo adrienthebo left a comment

Choose a reason for hiding this comment

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

LGTM! There are a couple of nice to have changes (mainly documentation) that would be good to add before merge, but I'm happy with this as-is.

lib/r10k/source/hash.rb Show resolved Hide resolved
lib/r10k/source/hash.rb Show resolved Hide resolved
lib/r10k/source/hash.rb Outdated Show resolved Hide resolved
lib/r10k/source/hash.rb Outdated Show resolved Hide resolved
@reidmv
Copy link
Contributor Author

reidmv commented Apr 22, 2020

Hmm. The failing test here looks like it originated from #952. Not sure what to do about that; I don't even get the failure locally. #952 passed tests before Ruby 2.7.0 was added to the matrix.

@reidmv
Copy link
Contributor Author

reidmv commented Apr 22, 2020

Tracked it down. The test failure is due to an rspec-mocks Ruby 2.7 bug in its #and_call_original method. See rspec/rspec-mocks#1306.

@reidmv
Copy link
Contributor Author

reidmv commented Apr 22, 2020

@adrienthebo ready for review, including bug workaround for rspec-mocks 3.9.1

@mwaggett
Copy link
Contributor

lgtm! did you want to squash any of your commits?

@reidmv reidmv force-pushed the exec-source branch 3 times, most recently from ad1257a to af0ce8c Compare April 22, 2020 19:28
@reidmv
Copy link
Contributor Author

reidmv commented Apr 22, 2020

Squashed down to three commits. 1) Improve hash source type, 2) add exec source type, and 3) the rspec-mocks test bug workaround.

mwaggett
mwaggett previously approved these changes Apr 22, 2020
Copy link
Contributor

@adrienthebo adrienthebo left a comment

Choose a reason for hiding this comment

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

We have a hard blocker for error handling, a minor change to YAML parsing, and some optional minor improvements. Let's get the blocker cleared out so we can get this merged, and then can do some refactoring in a followup PR.

spec/unit/cli_spec.rb Outdated Show resolved Hide resolved
lib/r10k/source/exec.rb Outdated Show resolved Hide resolved
lib/r10k/source/exec.rb Outdated Show resolved Hide resolved
lib/r10k/source/exec.rb Show resolved Hide resolved
lib/r10k/source/exec.rb Outdated Show resolved Hide resolved
@reidmv reidmv dismissed stale reviews from mwaggett via 8c4cf0e April 24, 2020 19:00
@reidmv reidmv force-pushed the exec-source branch 2 times, most recently from 9fcac92 to a933d9b Compare April 24, 2020 19:14
reidmv added 2 commits April 24, 2020 12:15
Two main improvements being made:

 - Include logging in hash source type
 - Split hash source #initialize from #environments
 - Add validation method for checking environment hashes

Logging is being added so that derived classes can use it.

Splitting the #initialize logic pertaining to creating environments and
and sending it into the #environments method instead allows child
classes to better control when (or if) side-effect actions are
triggered.

The validation method can be used to check if an environments hash is
valid. It only performs perfunctory validation today.
This new source type allows r10k to run a command, which should produce
a JSON or YAML hash, to enumerate environments that should exist.
Similar to Puppet's exec node terminus this becomes an easy extension
point to integrate r10k with custom external environment sources.
Co-Authored-By: Adrien Thebo <adrien@lagrange-automation.io>
@reidmv
Copy link
Contributor Author

reidmv commented Apr 24, 2020

Applied suggestions from code review, dropped rspec-mocks cli tests workaround, rebased on master.

@adrienthebo adrienthebo self-requested a review April 28, 2020 17:20
@adrienthebo adrienthebo dismissed their stale review April 28, 2020 17:20

Dismissing stale review.

Copy link
Contributor

@adrienthebo adrienthebo left a comment

Choose a reason for hiding this comment

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

LGTM!

@adrienthebo adrienthebo merged commit 8fe925b into puppetlabs:master Apr 28, 2020
@reidmv reidmv deleted the exec-source branch April 28, 2020 17:25
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