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

Remove puppet version pin from gemfile #3353

Merged
merged 7 commits into from
Nov 7, 2024

Conversation

alex501212
Copy link
Contributor

Since PE-35920 is resolved I have removed the puppet version pin in the gemfile

@alex501212 alex501212 requested a review from a team as a code owner October 22, 2024 16:04
@donoghuc
Copy link
Contributor

Looks like while that ticket is closed it is not actually resolved. We will probably want to address this issue. Specifically we will want a readable error when an unsupported Hiera lookup is attempted.

'kind' => 'bolt/pal-error',
'msg' => /Interpolations are not supported in lookups/
)
expect(result).to eq('test::interpolations data/common.yaml')
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to ensure bolt errors here. Returning a success here is a regression.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I completely forgot about #3213 I think you can update the test to validate that the default value is used in the lookup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this similar to other tests were we add options with a default value to the run_cli_json call?

    it 'accepts a default value' do
      options = { 'default_value' => 'default' }.to_json
      result  = run_cli_json(cli_command + %W[key=foo::bar::baz options=#{options}])
      expect(result).to eq('default')
    end

Copy link
Contributor

Choose a reason for hiding this comment

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

The core of the issue here is that we wanted to make it clear to users that in plan scope (specifically not in an apply block) there is not going to be the context (for example facts) necessary to resolve a hierachy that relies on that data for resolution. Previously we relied on a behavior in puppet whereby a lookup without necessary context would result in an error. Turns out that was not necessarily the correct assumption (see #3213 ). The path forward here is that a lookup attempted in plan scope (outside of an apply block) should be able to do a lookup without error using the defaults that the underlying puppet/hiera comes up with. We will need to update our tests to codify our expectations about what this behavior should be. Generally, does the test code in these fixtures https://github.com/puppetlabs/bolt/tree/main/spec/fixtures/hiera make sense to you? The idea is that we have some plan code and a bunch of hiera configurations used in the tests to validate assumptions about what lookups will resolve to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would https://github.com/puppetlabs/bolt/blob/main/spec/fixtures/hiera/modules/test/data/common.yaml need updated for this test? I'm not very familar with hiera so i'm wondering if theres any similar tests you could point me to?

Copy link
Contributor

Choose a reason for hiding this comment

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

The tests in this file largely cover the expectations for how lookups behave. The data itself in that file (common.yaml) should remain unchanged, the methods we use for doing the lookup (for example in top level plan scope) with the various configuration files we have there and the expected results will need to be refactored to ensure the defaults are expected.

This section of our docs does a decent job of explaining the basics. https://www.puppet.com/docs/bolt/latest/hiera.html You can review that and make sure that the new behavior still tracks with the docs.

@alex501212
Copy link
Contributor Author

after running lookup_spec.rb it looks like quite alot of tests are failing when using Puppet8:

rspec ./spec/integration/lookup_spec.rb:98 # lookup plan function with interpolations returns an error
rspec ./spec/integration/lookup_spec.rb:193 # lookup command without --plan-hierarchy looks up a value
rspec ./spec/integration/lookup_spec.rb:238 # lookup command without --plan-hierarchy looks up a value in the module hierarchy
rspec ./spec/integration/lookup_spec.rb:246 # lookup command without --plan-hierarchy errors with a missing key
rspec ./spec/integration/lookup_spec.rb:254 # lookup command without --plan-hierarchy looks up a value with a built-in backend
rspec ./spec/integration/lookup_spec.rb:262 # lookup command without --plan-hierarchy looks up a value with a custom backend
rspec ./spec/integration/lookup_spec.rb:281 # lookup command without --plan-hierarchy looks up the same value as a plan lookup
rspec ./spec/integration/lookup_spec.rb:204 # lookup command without --plan-hierarchy with interpolations looks up a value with facts
rspec ./spec/integration/lookup_spec.rb:212 # lookup command without --plan-hierarchy with interpolations looks up a value with vars
rspec ./spec/integration/lookup_spec.rb:220 # lookup command without --plan-hierarchy with interpolations looks up a value with a trusted fact
rspec ./spec/integration/lookup_spec.rb:231 # lookup command without --plan-hierarchy with interpolations with plan vars interpolated looks up a value with plan vars interpolated
rspec ./spec/integration/lookup_spec.rb:273 # lookup command without --plan-hierarchy with a missing backend returns an error
rspec ./spec/integration/lookup_spec.rb:310 # lookup command with --plan-hierarchy with invalid plan_hierarchy raises a validation error

@donoghuc
Copy link
Contributor

donoghuc commented Nov 1, 2024

Are you seeing those in CI? https://github.com/puppetlabs/bolt/actions/runs/11597999624/job/32350978455?pr=3353 I only see the three failures that are expecting error instead of using a default.

Copy link
Contributor

@gavindidrichsen gavindidrichsen left a comment

Choose a reason for hiding this comment

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

LGTM

@gavindidrichsen gavindidrichsen merged commit cca000f into puppetlabs:main Nov 7, 2024
23 checks passed
@alex501212 alex501212 deleted the remove-pin branch November 8, 2024 12:30
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