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

(GH-2815) Support plan-hierarchy lookups on the CLI #2859

Merged
merged 3 commits into from
Jun 10, 2021

Conversation

lucywyman
Copy link
Contributor

@lucywyman lucywyman commented Jun 1, 2021

This adds a new flag --plan-hierarchy to the bolt lookup CLI
command, which will perform a lookup as if in a plan outside an apply
block rather than in an apply block. The command optionally accepts
variable definitions for plan variable interpolation in hiera config.
The flag is mutually exclusive with targetting options, and either a
targetting option or --plan-hierarchy are required. The command
returns the bare value of the lookup, rather than a Result object.

This also updates the parallelize test to verify that return
statements return to only run on SSH infrastructure, since it keeps
falsely failing on Windows.

!feature

  • Lookup hiera plan_hierarchy values from the CLI (#2815)

    The bolt lookup command now has a --plan-hierarchy flag that will
    lookup values from Hiera's plan_hierarchy.

@lucywyman lucywyman force-pushed the GH-2815 branch 3 times, most recently from 48dad4e to e527377 Compare June 2, 2021 00:57
@lucywyman lucywyman marked this pull request as ready for review June 2, 2021 16:50
@lucywyman lucywyman requested review from hestonhoffman and a team as code owners June 2, 2021 16:50
lib/bolt/cli.rb Outdated Show resolved Hide resolved
lib/bolt/cli.rb Outdated Show resolved Hide resolved
lib/bolt/cli.rb Outdated Show resolved Hide resolved
lib/bolt/pal.rb Outdated Show resolved Hide resolved
spec/bolt/cli_spec.rb Outdated Show resolved Hide resolved
spec/bolt/cli_spec.rb Outdated Show resolved Hide resolved
@lucywyman lucywyman force-pushed the GH-2815 branch 4 times, most recently from b504387 to f87a231 Compare June 3, 2021 16:53
@lucywyman lucywyman requested a review from beechtom June 3, 2021 19:04
> **Note:** The `bolt lookup` and `Invoke-BoltLookup` commands only look up data
> using the `hierarchy` key in the Hiera configuration file. `plan_hierarchy`
> is not supported from the command line.
### Lookup up data from hierarchy key
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### Lookup up data from hierarchy key

Copy link
Contributor

@hestonhoffman hestonhoffman Jun 4, 2021

Choose a reason for hiding this comment

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

We don't mention the hierarchy key in this section outside of the section title. We've told the reader to learn more about Hiera and linked off to the Hiera docs, but I think we should add some references to where the hierarchy key is. I'll look for some opportunities do that below.

This command line section is a little difficult to parse right now because we've added a section on the plan_hierarchy key, but that key is only explained later in the plans section. The plan_hierarchy key was mentioned in a note that we're removing here, and I think I just missed this last time I reviewed. I think we should:

  • Move the plan_hierarchy lookup section under "Look up data in plans" (It can probably just go right at the end).
  • Add a sentence to the end of the "Look up data from the command line" section that says something like: "To look up data outside an apply block in a Bolt plan, see..."
  • Remove this section title (I added a piece that should send users in the right direction if they're
    looking for info on lookups outside apply blocks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmk, I really don't like the titles I gave the subheadings of Look up data on the command line, but I'm not sure if it's better to continue the theme of 'inside apply blocks / outside apply blocks' or to use more correct language around target context vs. plan context (or to even just say "from hierarchy" and "from plan_hierarchy")

documentation/hiera.md Outdated Show resolved Hide resolved
documentation/hiera.md Outdated Show resolved Hide resolved
documentation/hiera.md Outdated Show resolved Hide resolved
documentation/hiera.md Outdated Show resolved Hide resolved
documentation/hiera.md Outdated Show resolved Hide resolved
documentation/hiera.md Outdated Show resolved Hide resolved
Copy link
Contributor

@beechtom beechtom left a comment

Choose a reason for hiding this comment

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

Trying to set a reserved variable results in a stacktrace:

$ bolt lookup foo -t nix facts='{"os":{"name":"Windows"}}'                                                                                     1 ✘  9s   2.6.3 
Starting: install puppet and gather facts on ubuntu
Finished: install puppet and gather facts with 0 failures in 4.91 sec
Attempt to assign to a reserved variable name: 'facts' on node thomass-macbook-pro.local
Traceback (most recent call last):
	43: from /Users/tom/.rvm/gems/ruby-2.6.3/bin/ruby_executable_hooks:24:in `<main>'
	42: from /Users/tom/.rvm/gems/ruby-2.6.3/bin/ruby_executable_hooks:24:in `eval'
	41: from /Users/tom/git/bolt/vendor/bundle/ruby/2.6.0/bin/bolt:23:in `<main>'
	40: from /Users/tom/git/bolt/vendor/bundle/ruby/2.6.0/bin/bolt:23:in `load'
	39: from /Users/tom/git/bolt/exe/bolt:11:in `<top (required)>'
	38: from /Users/tom/git/bolt/lib/bolt/cli.rb:529:in `execute'
	37: from /Users/tom/git/bolt/lib/bolt/cli.rb:735:in `lookup'
	36: from /Users/tom/git/bolt/lib/bolt/outputter.rb:46:in `spin'
        ...

lib/bolt/pal.rb Show resolved Hide resolved
lib/bolt/pal.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@beechtom beechtom left a comment

Choose a reason for hiding this comment

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

  • Looks up values using the plan_hierarchy key
  • Interpolates variables passed on the CLI
  • Falls back to hierarchy if plan_hierarchy is not defined
  • Gracefully errors when interpolating variables that are not defined
  • Gracefully errors when using targeting options with --plan-hierarchy
  • Gracefully errors when defining reserved variables
    Note: Error is printed twice.
  • Sets variables in an 'apply' lookup
  • Shadows variables correctly in an 'apply' lookup

documentation/hiera.md Outdated Show resolved Hide resolved
This adds a new flag `--plan-hierarchy` to the `bolt lookup` CLI
command, which will perform a lookup as if in a plan outside an apply
block rather than in an apply block. The command optionally accepts
variable definitions for plan variable interpolation in hiera config.
The flag is mutually exclusive with targetting options, and either a
targetting option or `--plan-hierarchy` are required. The command
returns the bare value of the lookup, rather than a Result object.

This also updates the parallelize test to verify that `return`
statements return to only run on SSH infrastructure, since it keeps
falsely failing on Windows.

!feature

* **Lookup hiera plan_hierarchy values from the CLI** ([puppetlabs#2815](puppetlabs#2815))

  The `bolt lookup` command now has a `--plan-hierarchy` flag that will
  lookup values from Hiera's `plan_hierarchy`.
@hestonhoffman
Copy link
Contributor

hestonhoffman commented Jun 9, 2021

I wanted to make some edits outside of the changed lines, so I opened a PR here lucywyman#13

Summary of changes

Good instincts on moving the entire CLI section down in the page, I think it's a lot easier to follow now.

Moved the link to apply blocks up to the "Before you start" paragraph. Users who want to use Hiera with Bolt should probably already be fairly familiar with how Puppet and Bolt work together, so they're better educated about which context they need to use (inside/outside apply blocks).

For headings, let's just keep it consistent and go with inside / outside apply blocks. I know it feels a little weird cos it's the command line, but we've already established these contexts, so I think it makes it a bit easier for the user if we stay consistent and then go into more detail in the sections.

Updated the opening paragraph of CLI lookups to summarize both options.

@lucywyman
Copy link
Contributor Author

Docs updates look great, merged

@beechtom beechtom merged commit c3299e3 into puppetlabs:main Jun 10, 2021
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