Keystone V3: Fix time elements handling#1937
Conversation
cbe0258 to
a9dd45f
Compare
|
@kayrus Can you please create a corresponding issue that details the changes being made here? I've been flexible with combining multiple features/fixes in a single PR, but we still need issues to describe why changes are being made. |
|
@jtopjian updated |
|
Thank you! |
|
Build failed.
|
jtopjian
left a comment
There was a problem hiding this comment.
@kayrus I've suggested a simpler way of passing the time to the request body - let me know if you have any questions.
In addition, the acceptance tests are failing:
convenience.go:35: �[1;31mFailure in applicationcredentials_test.go, line 94: unexpected error �[0m�[1;33m"parsing time \"\"2119-01-01T12:12:12.000000\"\" as \"\"2006-01-02T15:04:05Z07:00\"\": cannot parse \"\"\" as \"Z07:00\""�[0m�[1;31m�[0m
It appears that the time format being returned is MilliNoZ, which needs the UnmarshalJSON method to be parsed correctly.
| } | ||
|
|
||
| // MarshalJSON converts our struct request into JSON. | ||
| func (opts CreateOpts) MarshalJSON() ([]byte, error) { |
There was a problem hiding this comment.
This can instead be implemented as:
// ToApplicationCredentialCreateMap formats a CreateOpts into a create request.
func (opts CreateOpts) ToApplicationCredentialCreateMap() (map[string]interface{}, error) {
b, err := gophercloud.BuildRequestBody(opts, "application_credential")
if err != nil {
return b, err
}
if opts.ExpiresAt != nil {
appCred := b["application_credential"].(map[string]interface{})
appCred["expires_at"] = opts.ExpiresAt.Format(gophercloud.RFC3339Milli)
b["application_credential"] = appCred
}
return b, nil
}This pattern is used in other areas of Gophercloud already.
In addition, we'll need to verify what time format Keystone is expecting. We're using Milli here, but MilliNoZ is being returned - that might cause some confusion and usage errors.
There was a problem hiding this comment.
My idea was to avoid body modification inside the To***CreateMap, but do this in MarshalJSON.
I clarified time format:
- Trusts accept and return RFC3339Milli, but response truncates milliseconds. Therefore for tests we need to truncate milliseconds.
- AppCreds accept and return RFC3339MilliNoZ. However for tests we need to truncate nanoseconds.
|
Build succeeded.
|
|
Build failed.
|
|
recheck |
|
Build succeeded.
|
|
@jtopjian PR is ready to review. I hope that |
|
I'd prefer not using There are a handful of other areas in Gophercloud that are using |
|
@jtopjian from my perspective Or in auth options Line 140 in b88f263 opts structure. Let me know if you agree, and I'll add this info into a contributor guide.
|
|
Here are the places, where you can use JSONMarshal as well: |
Two of those were written by me and I explicitly chose not to use
I agree that it could be used, but I have to respectfully request that it isn't. The The opposite is not true: you can't use In addition, There are a few occurrences of I understand Gophercloud leverages For now, I'd prefer having the rules of:
|
|
@jtopjian thanks for clarification. I'll use |
|
Build succeeded.
|
|
Build succeeded.
|
|
@jtopjian applied fixes |
Resolves #1938
This PR fixes issues in time handling.
Additionally I noticed that there is no error check in baremetal packages.