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.

Sign up for GitHub

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

Jump to bottom

Remove puppet version pin from gemfile #3353

Merged
merged 7 commits into from
Nov 7, 2024
Merged

Remove puppet version pin from gemfile #3353

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f5f43c2
remove puppet version pin from gemfile
alex501212 Oct 22, 2024
335bbb0
update test to return value instead of error
alex501212 Oct 30, 2024
0b92e55
revert last commit
alex501212 Oct 30, 2024
9990d84
remove ruby 2 tests and update lookup spec tests
alex501212 Nov 6, 2024
4b9c2fa
remove puppet7 as test version in apply_spec
alex501212 Nov 6, 2024
7d9ac15
remove some more ruby 2 tests
alex501212 Nov 6, 2024
edf79d6
updated docs
alex501212 Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,3 @@ local_gemfile = File.join(__dir__, 'Gemfile.local')
if File.exist? local_gemfile
eval_gemfile local_gemfile
end

# TODO: remove this pin once we solve PE-35920
gem "puppet", '~> 7.24'
8 changes: 2 additions & 6 deletions spec/integration/lookup_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,13 +94,9 @@

context 'with interpolations' do
let(:hiera_config) { File.join(project, 'hiera_interpolations.yaml') }

it 'returns an error' do
it 'returns a value' do
result = run_cli_json(cli_command + %w[key=test::interpolations])
expect(result).to include(
'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.

end
end

Expand Down
Loading