Default values on properties #148
Conversation
|
Can you put the base for this PR to #145? This way, I will only see the diff related to this change. Let me know if you need help with that. |
rambleraptor
force-pushed
the
prop-default
branch
3 times, most recently
from
ec75e71 to
f9272c3
Compare
api/type.rb
Outdated
| @@ -22,6 +22,7 @@ class Type < Api::Object::Named | |||
| module Fields | |||
| include Api::Object::Named::Properties | |||
|
|
|||
| attr_reader :default | |||
There was a problem hiding this comment.
Can you rename it to default_value to be more specific?
We have a similar concept in Terraform where the default value can be either a value defined here or at runtime by the API using another boolean field default_from_api. Once you change is in, I will update terraform to use your default_value and keep the default_from_api specific to Terraform.
There was a problem hiding this comment.
I think this could be type-checked without too much difficulty, and that might be a good idea. What do you think?
There was a problem hiding this comment.
You can steal my validation method I wrote for Terraform: https://github.com/GoogleCloudPlatform/magic-modules/blob/master/provider/terraform/property_override.rb#L74
Again, once you check-in this code, I will update the Terraform code to use the api.yaml defined default.
There was a problem hiding this comment.
Good call on the validation. All three nodes addressed.
|
I am a robot that works on MagicModules PRs! I built this PR into one or more PRs on other repositories, and when those are closed, this PR will also be merged and closed. (terraform removed because of unrelated change) |
api/type.rb
Outdated
| clazz = ::Symbol | ||
| else | ||
| raise "Update 'check_default_value_property' method to support " \ | ||
| "default value for type #{api_property.class}" |
There was a problem hiding this comment.
replace #{api_property.class} by #{self.class}
There was a problem hiding this comment.
Good catch!
(I also had a Rubocop fix)
rambleraptor
force-pushed
the
prop-default
branch
2 times, most recently
from
9ab70c7 to
4714509
Compare
| class Type < Api::Object::Named | ||
| # The list of properties (attr_reader) that can be overridden in | ||
| # <provider>.yaml. | ||
| module Fields | ||
| include Api::Object::Named::Properties | ||
|
|
||
| attr_reader :default_value |
There was a problem hiding this comment.
nit: it is common to have a unit associated with the value. like for example a timeout you could do: timeout_ms=long, or instead timeout=TimeUnit (where the type holds both the number and unit)
api/type.rb
Outdated
| def check_default_value_property | ||
| return if @default_value.nil? | ||
|
|
||
| if is_a?(Api::Type::String) |
There was a problem hiding this comment.
Use case...when...else instead
Change-Id: I480dba6f5e173b781091a5179919b8022769fb7e
rambleraptor
force-pushed
the
prop-default
branch
from
4714509 to
6970064
Compare
|
I am a robot that works on MagicModules PRs! Once this PR is approved, you can feel free to merge it without taking any further steps. |
Default values on properties + support for them in Ansible
[all]
Default values on properties
[terraform]
[puppet]
[puppet-dns]
[puppet-sql]
[puppet-compute]
[chef]