Skip to content

Conversation

@deads2k
Copy link
Contributor

@deads2k deads2k commented Mar 3, 2025

This will allow us to split the definition of API types from API client. While doing this, it is worthwhile to continue making it possible for those who wish to generate their own client it its entirety. This is useful for cases where we create a model and choose not to create a client for it. If we don't do that, then it isn't useful and isn't needed.

remaining

  • add --verify argument to confirm generation up to date
  • extract some common types to avoid duplication.

@deads2k deads2k force-pushed the select-generator branch from a0ac210 to 822570a Compare March 3, 2025 19:49
@deads2k deads2k changed the title [WIP] add --generators argument to allow specifying which generators to run [wip] make it possible to have separate serialization API types and the client to access them Mar 26, 2025
@deads2k deads2k changed the title [wip] make it possible to have separate serialization API types and the client to access them make it possible to have separate serialization API types and the client to access them May 20, 2025
@deads2k
Copy link
Contributor Author

deads2k commented May 20, 2025

/test all

@deads2k deads2k force-pushed the select-generator branch from ba2daee to 0cf699c Compare May 27, 2025 18:28
@vkareh vkareh requested a review from nimrodshn June 2, 2025 20:37
@nimrodshn nimrodshn requested a review from ahitacat June 3, 2025 14:16
@nimrodshn
Copy link
Collaborator

@deads2k Mind adding UTs? These usually provide helpful examples.

@deads2k
Copy link
Contributor Author

deads2k commented Jun 3, 2025

@deads2k Mind adding UTs? These usually provide helpful examples.

Ah, the unit tests aren't in the package using the standard go test patterns. I didn't even realize it had tests. Yeah, otherwise we're good?

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 5, 2025
@deads2k deads2k force-pushed the select-generator branch from 9c8cf33 to a9593d6 Compare June 5, 2025 20:39
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 5, 2025
}

switch generator {
case "builders-alias", "json-alias", "types-alias":
Copy link
Collaborator

@nimrodshn nimrodshn Jun 9, 2025

Choose a reason for hiding this comment

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

Mind adding a comment here please? [Explaining that these are run for the ocm-model explicitly.]

{{ $unmarshalTypeFunc := unmarshalTypeFunc .Type }}
{{ $readTypeFunc := readTypeFunc .Type }}
var {{ $marshalTypeFunc }} = {{ apiVersionPackage }}.{{ $marshalTypeFunc }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ultimately the this will generate the code of the SDK. Shouldn't we have the comments here?
i.e.
// {{ $marshalTypeFunc }} writes a value of the '{{ .Type.Name }}' type to the given writer.

@nimrodshn
Copy link
Collaborator

/lgtm
/hold for Victor's review
/assign @vkareh

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 9, 2025
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 9, 2025
@vkareh
Copy link
Member

vkareh commented Jun 9, 2025

This looks good, thanks @deads2k

/approve
/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 9, 2025
@openshift-ci
Copy link

openshift-ci bot commented Jun 9, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, vkareh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 9, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit 2defa0d into openshift-online:main Jun 9, 2025
5 checks passed
@deads2k
Copy link
Contributor Author

deads2k commented Jun 9, 2025

IOUs are here: #229

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants