Add FeatureFlagConfigurationSetting, SecretReferenceConfigurationSetting #16771
Conversation
…iated directly, or can be retrieved from the service
…hods for FeatureFlag
sdk/appconfiguration/azure-appconfiguration/azure/appconfiguration/_models.py
Outdated
Show resolved
Hide resolved
sdk/appconfiguration/azure-appconfiguration/azure/appconfiguration/_models.py
Outdated
Show resolved
Hide resolved
sdk/appconfiguration/azure-appconfiguration/azure/appconfiguration/_models.py
Show resolved
Hide resolved
| self.last_modified = kwargs.get("last_modified", None) | ||
| self.read_only = kwargs.get("read_only", None) | ||
| self.tags = kwargs.get("tags", None) | ||
| self.tags = kwargs.get("tags", {}) |
There was a problem hiding this comment.
Why we give default value {}?
Given the case user creates a ConfigSetting w/o tags, reads it and sends it back, will this change the value of tag?
| super(FeatureFlagConfigurationSetting, self).__init__(**kwargs) | ||
| self.key = key | ||
| if not key.startswith(self.key_prefix): | ||
| self.key = self.key_prefix + key |
There was a problem hiding this comment.
This is interesting. What's the behavior of other languages?
There was a problem hiding this comment.
This only happens when a user creates one in a Python program. On deserialization we only deserialize into FeatureFlagConfigurationSetting if the prefix and content_type are the correct value.
There was a problem hiding this comment.
I mean if user creates a FeatureFlagConfigurationSetting with key ".appconfig.featureflag/abc"
How can we know he/she wants ".appconfig.featureflag/abc" or ".appconfig.featureflag/.appconfig.featureflag/abc"?
There was a problem hiding this comment.
On the other hand do we want a user to have to create a feature flag by doing:
my_flag = FeatureFlagConfigurationSetting(".appconfig.featureflag/my_flag", enabled=True)or
my_flag = FeatureFlagConfigurationSetting("my_flag", enabled=True)There was a problem hiding this comment.
I don't have a strong opinion on this. Let's align the behavior with other languages. :)
There was a problem hiding this comment.
Talked with other languages. I will make the key prefix a constant and export it.
| if not new_value: | ||
| new_value = {} | ||
| self._enabled = new_value.get("enabled") or self._enabled | ||
| self._value = new_value |
There was a problem hiding this comment.
Looks like if we set value=None then get enabled, the value will be changed to {"enabled": self._enabled}?
There was a problem hiding this comment.
if new_value = None this will raise an AttributeError on line 196 (new_value.get("enabled")).
There was a problem hiding this comment.
{}.get('enabled')
returns None. :)
There was a problem hiding this comment.
Ahh, good catch. I will remove the if not new_value lines, then it will throw an AttributeError.
| @@ -147,8 +147,6 @@ def __init__(self, key, enabled, filters=None, **kwargs): | |||
| self._enabled = enabled | |||
| super(FeatureFlagConfigurationSetting, self).__init__(**kwargs) | |||
| self.key = key | |||
| if not key.startswith(self.key_prefix): | |||
There was a problem hiding this comment.
Do we want to check if key.startswith(self.key_prefix)?
There was a problem hiding this comment.
Do we still want to do this? I removed it to align with java and javascript/typescript.
There was a problem hiding this comment.
if not key.startswith(self.key_prefix):
raise ValueError. :)There was a problem hiding this comment.
There was some confusion in our discussion, the standard is to prefix no matter what. That is what Java and C# do, I will add the same to Python
| @property | ||
| def enabled(self): | ||
| # type: () -> bool | ||
| if self._value is None: |
There was a problem hiding this comment.
Not raise AttributeError("value parameter is expected to be a dictionary") if value is None?
There was a problem hiding this comment.
Are you suggesting that we raise that error if the user tries to set the value to None?
There was a problem hiding this comment.
If _value is not in right schema or key not startswith(self.key_prefix), it is not FeatureFlagConfigurationSetting any longer. Hence enabled & filters properties are invalid.
There was a problem hiding this comment.
self._value is the real value we store.
In a GETTER, we cannot change the value.
if self._value is None:
return Nonesame in
else:
self._value["enabled"] = self._enabledthe value can only be changed in a setter method which means user explicitly modify the value.
There was a problem hiding this comment.
And I would suggest we have a private method to validate if the value and key are valid. It will be called for multiple times. (any time user reads/write FeatureFlagConfigurationSetting specific properties, we need to validate it first.)
There was a problem hiding this comment.
To be clear, we will
- validate anytime
my_key.value = <new_value>andmy_key.enabled = <bool>occur, (validating on a set)? - Validate anytime a user tries to read the
valueorenabledproperties as well (validate on a get)? - If they are valid, we update the internal dictionary or enabled value
- if they are not valid (ie not a dictionary, not a boolean) we raise an error?
| def enabled(self, value): | ||
| # type: (bool) -> bool | ||
| if self._value is None: | ||
| self._value = {"enabled": value} |
There was a problem hiding this comment.
If self._value is None, it is not a FeatureFlagConfigurationSetting, right? :)
There was a problem hiding this comment.
FeatureFlagConfigurationSetting is determined by the key_prefix and content_type, The value can be anything, there is no service side validation.
There was a problem hiding this comment.
Do you mean enabled property is Nullable?
sdk/appconfiguration/azure-appconfiguration/azure/appconfiguration/_models.py
Outdated
Show resolved
Hide resolved
sdk/appconfiguration/azure-appconfiguration/azure/appconfiguration/_models.py
Outdated
Show resolved
Hide resolved
sdk/appconfiguration/azure-appconfiguration/azure/appconfiguration/_models.py
Outdated
Show resolved
Hide resolved
sdk/appconfiguration/azure-appconfiguration/azure/appconfiguration/_models.py
Outdated
Show resolved
Hide resolved
sdk/appconfiguration/azure-appconfiguration/azure/appconfiguration/_models.py
Outdated
Show resolved
Hide resolved
|
Btw, don't forget to update changelog. :) |
Add 'id' property to ClientEncryptionKeyResource (Azure#16771) * Add 'id' property to ClientEncryptionKeyResource * prettier fix * Mark operation as long running to fix warning.
No description provided.