Update the Cognitive SDK to use API Version 2024-10-01 from 2023-05-01
#27851
Conversation
|
Hey @liuwuliuyun, is this ready to be reviewed? |
|
Hi @stephybun, I'm currently addressing the issue with 3 new acceptance tests failing. I'll set it as ready for review as soon as I resolve these issues. |
There was a problem hiding this comment.
Thanks @liuwuliuyun, it looks like this upgrade contains a breaking change albeit a minor one. I'm going to mark this with the breaking change label for awareness and will check with the team whether we can proceed with this.
| if localAuthEnabled { | ||
| keys, err := client.AccountsListKeys(ctx, *id) | ||
| if err != nil { | ||
| // note for the resource we shouldn't gracefully fail since we have permission to CRUD it | ||
| return fmt.Errorf("listing the Keys for %s: %+v", *id, err) | ||
| } | ||
|
|
||
| if model := keys.Model; model != nil { | ||
| d.Set("primary_access_key", model.Key1) | ||
| d.Set("secondary_access_key", model.Key2) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Technically a breaking change since these were being populated when using 2023-05-01 even when local_authentication_enabled = false
Whether those keys were actually useful though is another question. Pointing this out for awareness.
| if localAuthEnabled { | ||
| keys, err := client.AccountsListKeys(ctx, *id) | ||
| if err != nil { | ||
| return fmt.Errorf("listing the Keys for %s: %+v", id, err) | ||
| } | ||
|
|
||
| if model := keys.Model; model != nil { | ||
| state.PrimaryAccessKey = pointer.From(model.Key1) | ||
| state.SecondaryAccessKey = pointer.From(model.Key2) | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you please update the documentation with a note below these properties with the explanation that these are only available if location_authentication_enabled is true
| if localAuthEnabled { | ||
| keys, err := client.AccountsListKeys(ctx, *id) | ||
| if err != nil { | ||
| // note for the resource we shouldn't gracefully fail since we have permission to CRUD it | ||
| return fmt.Errorf("listing the Keys for %s: %+v", *id, err) | ||
| } | ||
|
|
||
| if model := keys.Model; model != nil { | ||
| d.Set("primary_access_key", model.Key1) | ||
| d.Set("secondary_access_key", model.Key2) | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you please update the documentation with a note below these properties with the explanation that these are only available if location_auth_enabled is true
|
HI @stephybun , thanks for the review. I've updated the documentation to include reminders that the |
|
New testing results on datasource: |
There was a problem hiding this comment.
Thanks @liuwuliuyun, could you fix up the wording on the notes in the documentation? Once that's done this should be good to go.
Co-authored-by: stephybun <steph@hashicorp.com>
Co-authored-by: stephybun <steph@hashicorp.com>
Co-authored-by: stephybun <steph@hashicorp.com>
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Community Note
Description
PR Checklist
For example: “
resource_name_here- description of change e.g. adding propertynew_property_name_here”Changes to existing Resource / Data Source
Testing
These tests also failed in main branch, I will try to fix them in seperate PRs.
For state migrations please test the changes locally and provide details here, such as the versions involved in testing the migration path. For further details on testing state migration changes please see our guide on state migrations in the contributor documentation. -->
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
2024-10-01from2023-05-01[Update the Cognitive SDK to use API Version2024-10-01from2023-05-01#27851]This is a (please select all that apply):