Skip to content
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(HMS-1616): OpenAPI cleanup and examples #536

Merged
merged 1 commit into from
May 25, 2023

Conversation

lzap
Copy link
Member

@lzap lzap commented May 17, 2023

Continuation of #500 for all sources endpoints. I am keeping the deprecated endpoints untouched for now, let’s remove them in a separate PR.

I cannot continue working on upload_info until #534 is merged tho, so that is TBD.

@lzap lzap marked this pull request as ready for review May 23, 2023 13:53
@lzap lzap requested a review from a team as a code owner May 23, 2023 13:53
type SourceID struct {
SourceId string `json:"source_id"`
}
// See clients.Source
type SourceResponse struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we be able to add the provider type here?
We apparently have consumer for that info and it might be helpful even in the UI in the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to either do cleanup in this PR (removal of things) or nothing. I can probably put it back and create a ticket.

But whoever is using that number, it will change between prod/ephemeral and stage likely. It is a not good data source.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created https://issues.redhat.com/browse/HMS-1829

This is now ready for review and merge, I want to do followup for reservations and I am done!

AwsInfo: &clients.AccountDetailsAWS{AccountID: "78462784632"},
}

var SourceUploadInfoAzureResponse = payloads.SourceUploadInfoResponse{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused 🤔
This generates example where we have key azureinfo, but I don't see where the type is defined, but schema defines a key azure, so there is discrepancy in the key names, but I don't see where it comes from. Are we using the same types for The example response and the schema generating?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is defined as follows:

type SourceUploadInfoResponse struct {
          Provider  string                       `json:"provider"`
          AwsInfo   *clients.AccountDetailsAWS   `json:"aws" nullable:"true"`
          AzureInfo *clients.AccountDetailsAzure `json:"azure" nullable:"true"`
  }

Not sure what you mean by discrepancy, JSON can afford more compact names because every key has a context. Go types, however, are globally available. For this reason we have AzureInfo type, because Azure would be too broad term.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is a typical error I have seen many times now:

diff --git a/internal/payloads/sources_payload.go b/internal/payloads/sources_payload.go
index cb3dd40..2da271e 100644
--- a/internal/payloads/sources_payload.go
+++ b/internal/payloads/sources_payload.go
@@ -37,9 +37,9 @@ func NewListSourcesResponse(sourceList []*clients.Source) []render.Renderer {
 }
 
 type SourceUploadInfoResponse struct {
-       Provider  string                       `json:"provider"`
-       AwsInfo   *clients.AccountDetailsAWS   `json:"aws" nullable:"true"`
-       AzureInfo *clients.AccountDetailsAzure `json:"azure" nullable:"true"`
+       Provider  string                       `json:"provider" yaml:"provider"`
+       AwsInfo   *clients.AccountDetailsAWS   `json:"aws" nullable:"true" yaml:"aws"`
+       AzureInfo *clients.AccountDetailsAzure `json:"azure" nullable:"true" yaml:"azure"`
 }

This should fix it, pushing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That helped, thanks! 🚀

@lzap
Copy link
Member Author

lzap commented May 25, 2023

Thanks, as you can see no changes were done only examples, documentation and tags were added.

@lzap lzap force-pushed the openapi-next branch 2 times, most recently from de92b4d to d12cddc Compare May 25, 2023 13:16
@lzap
Copy link
Member Author

lzap commented May 25, 2023

I noticed a TODO comment in path.yaml that was no longer relevant, rebased.

@ezr-ondrej
Copy link
Member

let me regenerate the spec for you, it's just the description and we are ready to :shipit:

@ezr-ondrej ezr-ondrej merged commit 1f198bd into RHEnVision:main May 25, 2023
@ezr-ondrej
Copy link
Member

Thanks!

@lzap lzap deleted the openapi-next branch May 26, 2023 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants