feat(cdn): add cdn client, config, list command #1100
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rubenhoenle
reviewed
Nov 19, 2025
rubenhoenle
reviewed
Nov 19, 2025
rubenhoenle
reviewed
Nov 19, 2025
| return cmd | ||
| } | ||
|
|
||
| var sortByFlagOptions = []string{"id", "created", "updated", "origin-url", "status"} |
Member
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.
Member
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.
Contributor
Author
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?
rubenhoenle
reviewed
Nov 19, 2025
|
|
||
| func outputResult(p *print.Printer, outputFormat string, distributions []cdn.Distribution) error { | ||
| if distributions == nil { | ||
| distributions = make([]cdn.Distribution, 0) // otherwise prints null in json output |
Member
There was a problem hiding this comment.
That's a good catch indeed! Any chance we can solve this in a central place for all commands?
Contributor
Author
There was a problem hiding this comment.
Nice idea, I'll investigate this a bit, don't know much about go reflection yet.
Member
There was a problem hiding this comment.
Reflection is considered a no-no, see https://principles.schwarz/principles/engineering-principles/go/avoid-using-reflection
Contributor
Author
There was a problem hiding this comment.
With reflection I'd add this in Printer.OutputResult():
val := reflect.ValueOf(output)
if val.IsNil() {
// when passing a nil slice or map, JSON and YAML marshal to "null"
// so we need to create an empty slice or map to have "[]" or "{}" output instead
switch val.Kind() {
case reflect.Slice:
output = make([]any, 0)
case reflect.Map:
output = make(map[any]any)
default:
// do nothing
}
}The linked document suggests type switches, which would not work here.
Another solution without reflection, that comes to mind, would be to introduce specialized OutputResult functions, like OutputResultSlice/Map.
Member
There was a problem hiding this comment.
👍
Could be an option, please feel free to create a Jira ticket and propose a solution 😊
Co-authored-by: Ruben Hönle <Ruben.Hoenle@stackit.cloud>
Co-authored-by: Ruben Hönle <Ruben.Hoenle@stackit.cloud>
rubenhoenle
reviewed
Nov 19, 2025
rubenhoenle
reviewed
Nov 19, 2025
rubenhoenle
reviewed
Nov 19, 2025
Member
|
Btw, please always add the Jira issue ID to your PR description if possible :) |
cgoetz-inovex
commented
Nov 20, 2025
Merging this branch changes the coverage (3 decrease, 7 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
- test Min - test JoinStringMap - rm superfluous var for constant - rm file committed by accident - add nil checks when dereferencing pointers
Merging this branch changes the coverage (2 decrease, 8 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
rubenhoenle
reviewed
Dec 2, 2025
| } | ||
|
|
||
| // JoinStringMap concatenates the key-value pairs of a string map, key and value separated by kvSep, key value pairs separated by sep. | ||
| func JoinStringMap(m map[string]string, kvSep, sep string) string { |
Member
There was a problem hiding this comment.
it's fine, can stay here in this pkg
rubenhoenle
reviewed
Dec 2, 2025
| } | ||
|
|
||
| // JoinStringMap concatenates the key-value pairs of a string map, key and value separated by kvSep, key value pairs separated by sep. | ||
| func JoinStringMap(m map[string]string, kvSep, sep string) string { |
Member
There was a problem hiding this comment.
Suggested change
| func JoinStringMap(m map[string]string, kvSep, sep string) string { | |
| func JoinStringMap(m map[string]string, keyValueSeparator, separator string) string { |
Member
There was a problem hiding this comment.
I think it's more readable if you write it out, isn't it?
rubenhoenle
reviewed
Dec 2, 2025
| return JoinStringKeys(m, sep) | ||
| } | ||
|
|
||
| // JoinStringMap concatenates the key-value pairs of a string map, key and value separated by kvSep, key value pairs separated by sep. |
Member
There was a problem hiding this comment.
Suggested change
| // JoinStringMap concatenates the key-value pairs of a string map, key and value separated by kvSep, key value pairs separated by sep. | |
| // JoinStringMap concatenates the key-value pairs of a string map, key and value separated by keyValueSeparator, key value pairs separated by separator. |
rubenhoenle
reviewed
Dec 2, 2025
| func TestJoinStringMap(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| m map[string]string |
Member
There was a problem hiding this comment.
Suggested change
| m map[string]string | |
| input map[string]string |
Sorry, I'm nitpicking. But please use proper names.
rubenhoenle
reviewed
Dec 2, 2025
| ), | ||
| ), | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| ctx := context.Background() // should this be cancellable? |
Member
There was a problem hiding this comment.
Suggested change
| ctx := context.Background() // should this be cancellable? | |
| ctx := context.Background() |
We have it like this everywhere. And as of now I don't see any reason to change that. Any objections?
rubenhoenle
reviewed
Dec 2, 2025
|
|
||
| table := tables.NewTable() | ||
| table.SetHeader("ID", "REGIONS", "STATUS") | ||
| for i := range distributions { |
Member
There was a problem hiding this comment.
Suggested change
| for i := range distributions { | |
| for i, d := range distributions { |
Saves yourself the d := &distributions[i] line below, not?
rubenhoenle
reviewed
Dec 2, 2025
| table.SetHeader("ID", "REGIONS", "STATUS") | ||
| for i := range distributions { | ||
| d := &distributions[i] | ||
| joinedRegions := strings.Join(sdkUtils.EnumSliceToStringSlice(*d.Config.Regions), ", ") |
Member
There was a problem hiding this comment.
Take care of the potential nil pointer reference here please
rubenhoenle
reviewed
Dec 2, 2025
|
|
||
| // Returns the flag's value as a []string. | ||
| // Returns nil if flag is not set, if its value can not be converted to []string, or if the flag does not exist. | ||
| func FlagToStringArrayValue(p *print.Printer, cmd *cobra.Command, flag string) []string { |
Member
There was a problem hiding this comment.
Suggested change
| func FlagToStringArrayValue(p *print.Printer, cmd *cobra.Command, flag string) []string { | |
| func FlagToStringSliceValue(p *print.Printer, cmd *cobra.Command, flag string) []string { |
won't we call it slice in golang?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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)