-
Notifications
You must be signed in to change notification settings - Fork 15
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
Checks catalog openapi #536
Conversation
cba1dd9
to
99466dd
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.
Great!
I have noted some small issues to be corrected, together with other doubts.
type: :string, | ||
description: "Check Group, available when requiring a Flat Catalog" | ||
}, | ||
provider: Provider.SupportedProviders |
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.
Where is this SupportedProviders
specified?
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.
That's defined in lib/trento_web/openapi/schema/provider.ex:7
defmodule SupportedProviders do
@moduledoc false
OpenApiSpex.schema(%{
title: "SupportedProviders",
type: :string,
description: "Detected Provider where the resource is running",
enum: [:azure, :aws, :gcp, :unknown]
})
end
And now that I see it I think:
- should we consider
default
part of that list? - is
:unknown
actually a SupportedProvider?
I believe we can iterate more on these details.
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.
:unknown
is not supported in the checks catalog. We have in the code as an incoming provider in hosts, as we will have hosts that are not running in our known providers, and they will have this value.
So, in this SupportedProviders
I see it totally right.
default
is not part of this list, as I don't see any provider option as default
dbfaff1
to
9bc53ba
Compare
9bc53ba
to
ed5b4de
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.
Ready to merge from my side!
Added Checks Catalog OpenApi documentation.
vokoscreenNG-2022-05-16_18-28-34.mp4
The
flat
option fits a bit strangely in the open api spec.It might be worth considering making that more standard and actually making it a boolean.
Follows up #512 #522 #528