feat(SLO): download support for SLO API #1701
Conversation
| ) | ||
|
|
||
| // CreateConfig creates a config based on a given JSON payload and removes properties like the id (idPropertyKey), version and externalId | ||
| func CreateConfig(projectName string, data []byte, configType config.Type, idPropertyKey string) (config.Config, error) { |
There was a problem hiding this comment.
I would reduce the scope of this function to just removing unwanted entries. Than we could reuse it in every client. For example, the bucket client needs to delete:
r.Delete(bucketName)
r.Delete(status)
r.Delete(version)
r.Delete(updatable)The signature should look something like:
func Remove(data []byte, keys ... string) (json.JsonRaw, error)What's your opinion @arthurpitman ?
There was a problem hiding this comment.
I adapted it to only return the id and the modified json payload. The config creation is then again up to the segments/slos
There was a problem hiding this comment.
I adapted it again. Now it's also possible to use it for download buckets. I haven't touched it yet, because this would change the error logs
regarding the delete suggestion: I would not vote for that as it would only transform it basically from vertical to horizontal with no real value
| @@ -101,6 +101,10 @@ func GetDownloadCommand(fs afero.Fs, command Command) (cmd *cobra.Command) { | |||
| cmd.Flags().BoolVar(&f.onlySegments, "only-segments", false, "Only download segment configurations, skip all other configuration types") | |||
| } | |||
|
|
|||
| if featureflags.ServiceLevelObjective.Enabled() { | |||
| cmd.Flags().BoolVar(&f.onlySLOs, "only-slos", false, "Only download SLOs, skip all other configuration types") | |||
There was a problem hiding this comment.
For a user, it should be always slo-v2. Same as the type name.
As we also have slo in the classic endpoint, it could be confusing.
There was a problem hiding this comment.
done ✅
changed to only-slo-v2
There was a problem hiding this comment.
On second thought: plural or singular? only-slo-v2 vs only-slos-v2
Kirdock
force-pushed
the
feat/slo/download
branch
2 times, most recently
from
5294eb4 to
fab40e6
Compare
Kirdock
force-pushed
the
feat/slo/download
branch
from
fab40e6 to
2f5705e
Compare
tomazpu
added
the
release-notes
| } | ||
|
|
||
| // PrepareConfig returns the given id (idPropertyKey) and the JSON string without the given properties | ||
| func PrepareConfig(data []byte, structPointer any, deletedProperties []string, replaceParam string) (PreparedConfig, error) { |
There was a problem hiding this comment.
I would probably split this up into two methods, in a way, you're breaking the single responsibility principle. As you're getting the PreparedConfig and deleting("sanitizing") the downloaded data set.
Would it make sense to provide two methods, one returns ID. And another one to Sanitize the data set?
General feeling the method is complex and hard to read. Even breaking it up into private methods of steps that you do would make me happy.
There was a problem hiding this comment.
Done.
Tell me if you want to split it differently or if we should completely change the implementation
| Id string `json:"id"` | ||
| } | ||
|
|
||
| func TestCreateConfig(t *testing.T) { |
There was a problem hiding this comment.
I would also add some additional error test cases. Specially the field that I have in mind is structPointer as declared as any. The specific test for replaceParam.
There was a problem hiding this comment.
Wouldn't testing different variants of structPointer basically be like directly testing json.Unmarshal(data, structPointer) with any value?
There was a problem hiding this comment.
Anyway, I added some ✅
|
What this PR does / Why we need it:
Introduces the download command for SLOs