feat(cdn): add cdn client, config, list command #1100
Conversation
| return cmd | ||
| } | ||
|
|
||
| var sortByFlagOptions = []string{"id", "created", "updated", "origin-url", "status"} |
There was a problem hiding this comment.
Please directly use directly the API values: "id" "updatedAt" "createdAt" "originUrl" "status" "originUrlRelated"
This was wrong in the Jira story.
There was a problem hiding this comment.
Maybe there's also a enum generated in the SDK which holds these values. Then this would be the way to go because it will be kept up-to-date in the future without any efforts on our side.
There was a problem hiding this comment.
Searched for sortBy and originUrl etc. could not find an enum for this. The spec looks like it should generate an enum: https://github.com/stackitcloud/stackit-api-specifications/blob/83214868e6dbdc53d053cc07654542d8f34b5491/services/cdn/v1beta2/cdn.json#L1486
How could I start to investigate this further?
Co-authored-by: Ruben Hönle <Ruben.Hoenle@stackit.cloud>
Co-authored-by: Ruben Hönle <Ruben.Hoenle@stackit.cloud>
|
Btw, please always add the Jira issue ID to your PR description if possible :) |
| table.SetHeader("ID", "REGIONS", "STATUS") | ||
| for i := range distributions { | ||
| d := &distributions[i] | ||
| joinedRegions := strings.Join(sdkUtils.EnumSliceToStringSlice(*d.Config.Regions), ", ") |
There was a problem hiding this comment.
Take care of the potential nil pointer reference here please
There was a problem hiding this comment.
General question:
Is there a general approach? So should we check every pointer for nil?
The API spec for the response marks every step of distributions.config.regions as required.
I'll add nil checks but would be interested how our general approach is.
|
Something general: CDN is now GA ( I'm open for discussion if you want to directly integrate the GA version of the API within this PR or if you want to do it in a follow-up PR. In that case please create a Jira work item for it and bring it up in the refinement 😊 |
| }, | ||
| }, | ||
| expected: ` | ||
| ID │ REGIONS │ STATUS |
There was a problem hiding this comment.
some question I have in my mind here: Would it make sense maybe to create some abstraction for table outputs (or mocks) so we can properly test the table output without having to copy formatted tables into our unit tests.
Don't get me wrong, I absolutely (!!) love the idea that we will test the table output for all subcommands in the future, but these formatted tables copied into our unit test files could become a pain to maintain 😄
There was a problem hiding this comment.
Also thought about that, especially that the strings are padded with spaces is a bit painful.
For maintaining the current solution I used a breakpoint in the failure branch and evaluatedbuf.String() in the debugger, copying the result into the test and adding placeholders.
I'd add this point to our discussion to also test the run functions. There are a lot of possibilities how we could solve this. We could write a matcher library for this output or could go as far as the stuff mentioned in https://research.swtch.com/testing . Or something in between.
Co-authored-by: Ruben Hönle <Ruben.Hoenle@stackit.cloud>
- replace single char names with more descriptive ones - remove predefined mod functions - deduplicate ParseOriginRequestHeaders and test it - deduplicate ParseGeofencing and test it - add test FlagTo* funcs
|
This PR was marked as stale after 7 days of inactivity and will be closed after another 7 days of further inactivity. If this PR should be kept open, just add a comment, remove the stale label or push new commits to it. |
Description
Issue: STACKITCLI-70
Checklist
make fmtmake generate-docs(will be checked by CI)make test(will be checked by CI)make lint(will be checked by CI)