Add Snapshot selector support #17
Conversation
| LabelFilter *string `json:"labelFilter,omitempty"` | ||
| KeyFilter *string `json:"keyFilter,omitempty"` | ||
| LabelFilter *string `json:"labelFilter,omitempty"` | ||
| SnapshotName *string `json:"snapshotName,omitempty"` |
There was a problem hiding this comment.
why is this not named xxxFilter? sounds inconsistent
There was a problem hiding this comment.
This is discussed in the design, snapshotFilter sounds not reasonable.
| @@ -71,21 +71,21 @@ func verifyObject(spec acpv1.AzureAppConfigurationProviderSpec) error { | |||
|
|
|||
| // Check if feature flag label filters are valid | |||
| for i := range spec.FeatureFlag.Selectors { | |||
| if spec.FeatureFlag.Selectors[i].LabelFilter != nil && hasNonEscapedValueInLabel(*spec.FeatureFlag.Selectors[i].LabelFilter) { | |||
| return loader.NewArgumentError("spec.featureFlag.selectors", fmt.Errorf("non-escaped reserved wildcard character '*' and multiple labels separator ',' are not supported in label filters")) | |||
| err = verifySelectorObject(spec.FeatureFlag.Selectors[i]) | |||
There was a problem hiding this comment.
FeatureFlag is verifying input the same way asa Configuration.Selectors. Does this mean FeatureFlag supports Snapshot filter now?
There was a problem hiding this comment.
Yes, it will filter out all the feature flag type key-values in the specified snapshot
|
|
||
| if *snapshot.CompositionType != azappconfig.CompositionTypeKey { |
There was a problem hiding this comment.
why only accept 'key' type and not allowing key+lable?
There was a problem hiding this comment.
This is by design, it's consistent with other providers. support key+label type would cause ambiguity problem, which label would be populated is not definitive
| }, | ||
| } | ||
|
|
||
| Expect(verifyObject(configProviderSpec).Error()).Should(Equal("spec.configuration.selectors: set both keyFilter and snapshotName in one selector causes ambiguity, only one of them should be set")) |
There was a problem hiding this comment.
'set both keyFilter...' -> 'setting both keyFilter...'
| @@ -396,6 +396,7 @@ var _ = Describe("AppConfiguationProvider controller", func() { | |||
| ctx := context.Background() | |||
| providerName := "test-appconfigurationprovider-8" | |||
| configMapName := "file-style-configmap-to-be-created-2" | |||
| wildcard := "*" | |||
There was a problem hiding this comment.
Can user set wildcard in SnapshotName filed?
There was a problem hiding this comment.
We allow it , but we don't treat it as wildcard, it's just a normal character
| err := json.Unmarshal([]byte(*setting.Value), &out) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("Failed to unmarshal feature flag settings: %s", err.Error()) | ||
| if setting.ContentType != nil && |
There was a problem hiding this comment.
If user set snapshot in feature flag section，what's the value of setting.ContentType when feature flags from snapshot?
There was a problem hiding this comment.
Same with what we filtered out by key/lable filters
| } | ||
| } | ||
| } else { | ||
| snapshot, err := client.GetSnapshot(ctx, *filter.SnapshotName, nil) |
There was a problem hiding this comment.
Can we add snapshot status check since it can be archived? And what's expected behavior if the snapshot expire after some interval?
There was a problem hiding this comment.
Archived snapshots are still available to be used.
| @@ -693,7 +718,10 @@ func getFeatureFlagFilters(acpSpec acpv1.AzureAppConfigurationProviderSpec) []ac | |||
| if acpSpec.FeatureFlag != nil { | |||
| featureFlagFilters = deduplicateFilters(acpSpec.FeatureFlag.Selectors) | |||
| for i := 0; i < len(featureFlagFilters); i++ { | |||
| featureFlagFilters[i].KeyFilter = FeatureFlagKeyPrefix + featureFlagFilters[i].KeyFilter | |||
| if featureFlagFilters[i].KeyFilter != nil { | |||
There was a problem hiding this comment.
If user set snapshot in feature flag section, what is expected behavior? Ignoring all featureFlag.selector.snapshot ?
There was a problem hiding this comment.
No, we go ahead to use it, get the snapshot, and retrieve the feature flags settings from it.
Add Snapshot selector support