-
Notifications
You must be signed in to change notification settings - Fork 105
Pull newer fleet schema #834
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
Conversation
f8c264f
to
5f669f9
Compare
This is largely unworkable in the published spec. There's an internal discussion happening already about enforcing actual operation ids. Are any of the other changes causing issues with code generation? In general we should be aiming to fix these upstream as much as possible since ideally we're not the only consumer of the API spec. I'm not sure how much you can ignore the operationID issue at the moment, but I certainly wouldn't put effort into rewriting those since we'll be fixing those in the source. |
Regarding opIDs, nothing major went into it. just a big list with some hardcoded values roughly similar to what was there before. See here I added a discriminator to the output union here but we can definitely omit it and just deserialize a Various nested structs have refs made as we use them, otherwise the only real "issue" is some of the complicated anyOfs that we really don't care about in the implementation. For example: the kafka output properties compression_level, connection_type, password, and username look roughly like this:
This definitely feels like the verification refs are ending up in the final schema, at a minimum having both anyOf and oneOf is likely wrong. For our purposes, we really don't care just give the raw value and see if it matches the user's input (once kafka is implemented). The same is somewhat happening to the various package_policy vars fields. They are correctly oneOf bool, number, string, arrays, or secretRef. We could handle it appropriately, and check each of the 6 different possibilities, or just change it into a map[string]any and do how we were doing before. |
Two errors that actually do look like they might be wrong upstream:
These are both listed as --- edit Might be a more prevalent problem that I previously thought. |
188d81e
to
d1a454a
Compare
Ok. We're passing tests as they were previously written. Here's a detailed example of one of the nullable issues:
Now we could use the oapi nullable types to get around this potentially, Here is the list of changes necessary to get past all the tests in the workflow. -- edit |
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.
Are you able to open issues in the Kibana repo for the spec issues? The nullable behaviour at least sounds like a server bug?
It'd be nice to link the various transforms we're applying here back to an upstream issue so we can ideally track whether it continues to be required more easily.
Sure, let me go through the transforms again and see what I can open in Kibana. |
I think I'm about done here. I gave the nullable types a test-run, and didn't seem to get much in the way of usefulness. At best, all we would gain is the ability to differentiate tf.Unknown to api.Unset. So long as the fields are properly nullable, not sending an unset null vs always sending null is effectively the same thing. |
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. Honestly not really actually, I intensely dislike that we have this code. The solution though, that looks good.
There's a lot of work here, it's really appreciated. It not only helps us keep track with the Fleet API changes but will be helpful with the wider Kibana spec in the future.
👍
* wip * lint * fix downgrade * fresh pull and documentation * changelog --------- Co-authored-by: Toby Brain <[email protected]>
So the Fleet schema is getting pulled into Kibana's oas_docs folder now. Notable changes include:
/path/to/some/method#0
which not only break the generated code, are horrible to read.application/json
is nowapplication/json; Elastic-Api-Version=2023-10-31
So far, I've updated the outputs paths, but tldr, all the paths need nested structs broken out and rewritten operationIDs.
I'm also hoping to eventually pull this generator down a folder and use it to spit out the other generated paths as they come available. SLOs, dataviews, spaces, and some of alerting are already there iirc.