-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(Partner Center Sell): re-gen service after recent API definition changes #358
Conversation
The docs were updated as well under the https://test.cloud.ibm.com/apidocs/partner-center-sell?code=go link |
@@ -3155,13 +3045,13 @@ type CatalogHighlightItem struct { | |||
Description *string `json:"description,omitempty"` | |||
|
|||
// The description about the features of the product in translation. | |||
DescriptionI18n map[string]string `json:"description_i18n,omitempty"` | |||
DescriptionI18n map[string]interface{} `json:"description_i18n,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The map[string]interface{}
type results from the definition of the CatalogProductHighlightDescriptionI18n schema:
CatalogProductHighlightDescriptionI18n:
type: object
description: The description about the features of the product in translation.
additionalProperties: true
Are you sure that using interface{}
for the map value type is correct? Using interface{} types just makes it harder for users to deal with the values as a type assertion would be needed. If you know that the values will be strings, then you could change additionalProperties to be:
CatalogProductHighlightDescriptionI18n:
type: object
description: The description about the features of the product in translation.
additionalProperties:
type: string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had issue with passing custom data, in terraform this change seems to solve that problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean specifically by "custom data"? Does the service support values of any type (string, boolean, integer, object, float, etc.)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Readded types
@@ -349,43 +279,52 @@ var _ = Describe(`PartnerCenterSellV1 Integration Tests`, func() { | |||
|
|||
catalogHighlightItemModel := &partnercentersellv1.CatalogHighlightItem{ | |||
Description: core.StringPtr("highlight desc"), | |||
DescriptionI18n: map[string]string{"key1": "desc"}, | |||
DescriptionI18n: map[string]interface{}{"key1": "testString"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wherever you see a mock value like "testString"
being used in the generated integration tests or examples code, that is an opportunity to define a realistic example value in the API definition. The added benefit is that these example values will be used in the generated examples and that means that the programming examples will look "similar" (other than language-specific differences) across the languages and will help promote easier understanding of the API.
Here's a tutorial video that discusses this:
https://secure.video.ibm.com/channel/23887899/playlist/651457/video/133486087
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its true, but right now we do not plan to go into other languages. The main use case for this service is still terraform. Somewhere in the docs we should add the following warning:
Note - Intended for internal use only. This resource is strictly experimental and subject to change without notice.
(it has been added to related terraform docs as well.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could perhaps update the service table in README.md to include Experimental
after the service name:
You could perhaps also add Note: the Partner Center Sell service is intended for internal use only. This service is strictly experimental and subject to change without notice.
just after the table of "moved" services:
(right before Prerequisites)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its true, but right now we do not plan to go into other languages.
Even if you are only going to produce a Go SDK and Terraform provider, the ability to generate the integration tests and examples code when a change is made to the API (without needing to make manual changes) will save you lots of time in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't mind the example fix would go to the next PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed my approval until some of the comments have been reviewed and a decision is made as to whether or not to address them.
…d to the service Signed-off-by: Peter Harasztia <[email protected]>
Signed-off-by: Peter Harasztia <[email protected]>
Signed-off-by: Peter Harasztia <[email protected]>
Signed-off-by: Peter Harasztia <[email protected]>
Signed-off-by: Peter Harasztia <[email protected]>
Signed-off-by: Peter Harasztia <[email protected]>
Signed-off-by: Peter Harasztia <[email protected]>
Signed-off-by: Peter Harasztia <[email protected]>
Signed-off-by: Peter Harasztia <[email protected]>
Signed-off-by: Peter Harasztia <[email protected]>
Signed-off-by: Peter Harasztia <[email protected]>
Co-authored-by: Phil Adams <[email protected]> Signed-off-by: Peter Harasztia <[email protected]>
Co-authored-by: Phil Adams <[email protected]> Signed-off-by: Peter Harasztia <[email protected]>
Signed-off-by: Peter Harasztia <[email protected]>
a017a8b
to
9a9e326
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of minor changes needed. Re: the other questions, I don't think they're showstoppers, so just let me know if you want to ignore for now and I can merge.
Signed-off-by: Peter Harasztia <[email protected]>
…on comments Signed-off-by: Peter Harasztia <[email protected]>
Signed-off-by: Peter Harasztia <[email protected]>
Signed-off-by: Peter Harasztia <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting closer. Just need to resolve the default service URL and I was just curious about the change to the CM int test.
Signed-off-by: Peter Harasztia <[email protected]>
Signed-off-by: Peter Harasztia <[email protected]>
The cm test was a mistake removed it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
## [0.69.2](v0.69.1...v0.69.2) (2024-10-07) ### Bug Fixes * **Partner Center Sell:** re-gen service after recent API definition changes ([#358](#358)) ([36d86ae](36d86ae))
🎉 This PR is included in version 0.69.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
## [0.69.2](IBM/platform-services-go-sdk@v0.69.1...v0.69.2) (2024-10-07) ### Bug Fixes * **Partner Center Sell:** re-gen service after recent API definition changes ([IBM#358](IBM#358)) ([36d86ae](IBM@36d86ae)) Signed-off-by: manu.k.m <[email protected]>
PR summary
PR Checklist
Please make sure that your PR fulfills the following requirements:
Current vs new behavior
Does this PR introduce a breaking change?
Other information