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

(PDK-512) Add support for simple_get_filter #37

Merged
merged 2 commits into from
Mar 20, 2018
Merged

(PDK-512) Add support for simple_get_filter #37

merged 2 commits into from
Mar 20, 2018

Conversation

da-ar
Copy link

@da-ar da-ar commented Mar 20, 2018

No description provided.

@da-ar da-ar requested a review from DavidS March 20, 2018 12:35
Copy link
Contributor

@DavidS DavidS left a comment

Choose a reason for hiding this comment

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

One mismatch with the spec, and two minor cleanups.

🎉

@@ -3,9 +3,9 @@

RSpec.describe Puppet::ResourceApi::SimpleProvider do
let(:context) { instance_double('Puppet::ResourceApi::BaseContext', 'context') }
let(:provider_class) do
let(:provider_class) do # here...
Copy link
Contributor

Choose a reason for hiding this comment

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

there?

unknown_features = definition[:features] - supported_features
Puppet.warning("Unknown feature detected: #{unknown_features.inspect}") unless unknown_features.empty?


Copy link
Contributor

Choose a reason for hiding this comment

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

whitespace should be fixed in previous commit

@@ -7,7 +7,12 @@ module Puppet::ResourceApi
class SimpleProvider
def set(context, changes)
changes.each do |name, change|
is = change.key?(:is) ? change[:is] : (get(context) || []).find { |r| r[:name] == name }
is = if context.feature_support?('simple_get_filter')
change.key?(:is) ? change[:is] : (get(context, [name]) || []).first
Copy link
Contributor

Choose a reason for hiding this comment

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

according to the spec, get(context, [name]) is allowed to return more than one resource, and the first is not necessarily the one we were asking for. This still needs the same .find { |r| r[:name] == name } as below.

@DavidS DavidS merged commit 3d1d547 into puppetlabs:master Mar 20, 2018
@da-ar da-ar deleted the simple_get_filter branch March 23, 2018 14:40
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.

2 participants