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

Convert more set_or_returns to proper properties #6950

Merged
merged 2 commits into from
Mar 7, 2018
Merged

Conversation

tas50
Copy link
Contributor

@tas50 tas50 commented Mar 5, 2018

Just modernizing our resources so we can start to add descriptions to them for the documentation

Signed-off-by: Tim Smith tsmith@chef.io

@tas50 tas50 requested a review from a team March 5, 2018 18:21
Just modernizing our resources so we can start to add descriptions to them for the documentation

Signed-off-by: Tim Smith <tsmith@chef.io>
@@ -18,6 +18,7 @@
#

require "chef/resource"
require "chef/provider/cron" # do not remove. we actually need this below
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have a pile of cron constants in the provider

)
end

property :time, Symbol, equal_to: Chef::Provider::Cron::SPECIAL_TIME_VALUES
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically we didn't restrict this to a symbol, but we expected it to be a symbol in the equal_to so this is the same

)
end
property :append, [ TrueClass, FalseClass ], default: false
property :system, [ TrueClass, FalseClass ], default: false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This lacked a default before, but it seems like the default was in fact false

identity_attr :target_file

state_attrs :to, :owner, :group
state_attrs :owner # required since it's not a property below
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comes from Chef::Mixin::Securable which doesn't use properties so no state set

end
property :target_file, String, name_property: true, identity: true
property :to, String
property :link_type, [String, Symbol], coerce: proc { |arg| arg.kind_of?(String) ? arg.to_sym : arg }, equal_to: [ :symbolic, :hard ], default: :symbolic
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This had no type before, but it could be string or symbol so lets enforce that now

property :target_file, String, name_property: true, identity: true
property :to, String
property :link_type, [String, Symbol], coerce: proc { |arg| arg.kind_of?(String) ? arg.to_sym : arg }, equal_to: [ :symbolic, :hard ], default: :symbolic
property :group, [String, Integer], regex: [Chef::Config[:group_valid_regex]]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

group/user had no type before, but our spec checks integers so clearly those should be here

Copy link
Contributor

@thommay thommay left a comment

Choose a reason for hiding this comment

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

tenor-33574385

@thommay
Copy link
Contributor

thommay commented Mar 6, 2018

genuine red though: https://travis-ci.org/chef/chef/jobs/349650692#L1668

We're not longer initializing user to nil and instead we're letting
property handle it. This user => nil statement isn't the actual test
here, but was just a workaround for the funky behavior.

Signed-off-by: Tim Smith <tsmith@chef.io>
@tas50 tas50 merged commit 6e0c71d into master Mar 7, 2018
@tas50 tas50 deleted the more_properties branch March 7, 2018 18:02
@lock
Copy link

lock bot commented May 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants