[glance]: handle various image struct member types #3159
Conversation
|
I'd really love to have @stephenfin's eyes on this first |
|
@stephenfin any updates? |
|
This is required to fix the terraform-provider-openstack/terraform-provider-openstack#1703 |
stephenfin
left a comment
There was a problem hiding this comment.
God I hate the Glance API 😞
Two asks for comments inline but I'm otherwise good. Sorry for the delay: I missed the ask.
openstack/image/v2/images/results.go
Outdated
| switch t := s.Hidden.(type) { | ||
| case nil: | ||
| r.Hidden = false | ||
| case bool: | ||
| r.Hidden = t | ||
| case string: | ||
| r.Hidden, err = strconv.ParseBool(t) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed to parse Hidden %q: %v", t, err) | ||
| } | ||
| default: | ||
| return fmt.Errorf("Unknown type for Hidden: %v (value: %v)", reflect.TypeOf(t), t) | ||
| } | ||
|
|
There was a problem hiding this comment.
How is this happening. This should always be a boolean since that's how it's stored in the DB (source) and the API is simply using exactly what is stored there (source).
Sorry, you explained this in the issue. It's been a while 😅 Would it be possible to get a comment here explaining what's going on for future us (without needing to search though GitHub Issues)
| "updated_at": "2014-05-05T17:15:11Z", | ||
| "visibility": "public", | ||
| "os_hidden": false, | ||
| "os_hidden": "False", |
There was a problem hiding this comment.
This could do with a note about what we're simulating here also...
520b250 to
e96c3c2
Compare
3815ffc to
d4aeae5
Compare
d4aeae5 to
5411d4a
Compare
| r.Name, err = assert.String(s.Name, "Name") | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Is this necessary? What's the difference between this and regular json unmarshalling with omitempty?
Ditto all other uses of String below.
| @@ -0,0 +1,70 @@ | |||
| package assert | |||
There was a problem hiding this comment.
These aren't assertions. These are format conversions. jsonhelpers maybe?
|
|
||
| const errf = "unknown type for %s: %T (value: %v)" | ||
|
|
||
| func Int(v any, name string) (int, error) { |
| return 0, fmt.Errorf(errf, name, v, v) | ||
| } | ||
|
|
||
| func Int64(v any, name string) (int64, error) { |
There was a problem hiding this comment.
If you made NumberOrString generic on the return type you wouldn't need this function at all.
type ConvertibleNumber interface { int | int64 | float32 | float64 }| return 0, fmt.Errorf(errf, name, v, v) | ||
| } | ||
|
|
||
| func String(v any, name string) (string, error) { |
There was a problem hiding this comment.
I'm not sure this is necessary.
| return "", fmt.Errorf(errf, name, v, v) | ||
| } | ||
|
|
||
| func Bool(v any, name string) (bool, error) { |
|
@stephenfin @pierreprinetti can anyone take a look at this PR? 😇 thanks! |
|
I didn't expect that anyone else is interested in this PR. |
It looks like there's a few outstanding comments from @mdbooth that need addressing/response first? |
|
Any updates? @stephenfin @pierreprinetti @mdbooth |
This is still true? |
|
@kayrus are you still working on this PR? Looks like there's a few comments that need to be resolved |
|
@WizardBit not at the moment. feel free to submit a new PR. |
Unfortunately I am unfamiliar with Go, but thank you for the update |
|
Is this pull request dead? I'm unfamiliar with Go aswell but this is still a affecting problem for our Terraform users. |
As kayrus said, this needs someone else to pick up the work until when they've time to work on it again. |
Fixes #3147
Links to the line numbers/files in the OpenStack source code that support the
code in this PR:
[PUT URLS HERE]