-
Notifications
You must be signed in to change notification settings - Fork 1
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
PC-9915: Change SDK objects API #90
Conversation
229127d
to
022c632
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.
As agreed, I only took a general look without testing or trying to use the client. But I like the new shape! It is simple to understand and use.
I left a couple of minor comments and a few questions mainly to think about / discuss rather than change.
3c7e821
to
acbea63
Compare
acbea63
to
08a490a
Compare
5592074
to
786ff36
Compare
Co-authored-by: krolik <szymon@nobl9.com>
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.
🍊
Premise
Operating on a generic
map[string]interface{}
doesn't lend to a great devx, If you want to define your objects in code and then usesdk.Client
to apply them, you'd need to convert them somehow tosdk.AnyJSONObject
(most likely through a round trip for marshaling).Furthermore the intermediate representation of
GenericObject
along withAPIObjects
model was initially created to serve Ingest's purposes, and does not fit well into standard API user workflow.Objects often shared code, like
Metadata
and only used parts of its definition (like only name and project), which was introducing ambiguity as to what exactly does the object definition permit to be defined.What's changed?
Manifest
manifest
package is now only defining the absolute core of our schema, things we never expect to change and take for granted. It definesObject
interface which must be implemented by all objects across all API versions. It's the building block we can operate on at the highest level of abstraction.v1alpha
Metadata
orObjectHeader
, instead they explicitly list the fields and define their own flavours of metadatamanifest.Object
,manifest.ProjectScopedObject
andv1alpha.ObjectContext
interfaces is auto generated to reduce boilerplate work, the generator script is in-house and can be adjusted over time (scripts/generate-object-impl.go
. The auto generated code lives under<object_filename>_object.go
.APIObjects
has been moved to Ingest and will be eventually removed entirely in favour of[]manifest.Object
whilegenericToXXX
methods were removed. The parsing function located atparser.go
now uses generics to decode each objects into its concrete implementationOrganizationInformation
definition has been moved to IngestClient
sdk.Client
operates onmanifest.Object
instead of the oldmap[string]interface{}
, this means it both accepts and returns parsedmanifest.Object
sdk.Client
now sets organization automatically for all the objects it accepts in apply and delete methodsDefinitions
definitions
package now handles the decoding process on its own, it no longer relies on a bloated k8s yaml import. It is able to decode both yaml and json formats into specificmanifest.Object
. I used https://github.com/goccy/go-yaml as it offers much greater flexibility in decoding then https://github.com/go-yaml/yaml.definitions
package no longer requires implicitMetdataAnnotations
and it only annotates the objects with manifest source, the Project is set by the SDK users (like sloctl) and Organization is set bysdk.Client
What's to come
manifest.Version
enum in objects definitionsMetadata