Skip to content

Conversation

@atiratree
Copy link
Member

This PR adds complete types with documentation for all Kubernetes/OpenShift/KubeVirt objects (please see old #1773 for more details).

The types are generated from OpenAPI specification taken from

This PR uses openapi-generator instead of autorest which was mentioned in the last PR.

I found many problems with autorest:

  • unstable (crashes on quite a lot of nodejs versions)
  • slow
  • incorrect parsing of kubevirt's swagger.json.

openapi-generator worked quite well for me, but I still had to do few necessary changes in post processing phase.

Algorithm:

  • download current API Spec file (or add a custom URL to ENV variable)
  • process the spec file (remove paths key to inhibit API generation )
  • run openapi-generator which should generate just the models
  • for each generated file:
    • remove unnecessary functions (like IoK8sApiCoreV1SecretFromJSON)
    • remove unnecessary imports (like import { exists, mapValues } from '../runtime';) for these functions
    • import each file specifically instead of from './' to prevent circular dependencies
    • replace occurrences for example of IoK8sApiCoreV1Pod with V1Pod
  • run ESLint to enforce our formatting rules onto the generated files

@spadgett @alecmerdler @christianvogt @vojtechszocs

@openshift-ci-robot openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 25, 2019
@openshift-ci-robot
Copy link
Contributor

Hi @suomiy. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 25, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: suomiy
To complete the pull request process, please assign alecmerdler
You can assign the PR to them by writing /assign @alecmerdler in a comment when ready.

The full list of commands accepted by this bot can be found 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

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 removed some package names from generated object names because the names would be too long (as discussed in #1773).

There are some conflicts between the different packages so I had to leave some in form of short prefixes.

Please give me some feedback what are your thoughts on this. And which packages should be namespaced and what prefixes to use.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also kubevirt and openshift have some conflicts between them, but I have not done anything about that because they live in separate folders and the user can import them separately.

@spadgett
Copy link
Member

/assign @alecmerdler

@atiratree atiratree mentioned this pull request Jun 27, 2019
1 task
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 9, 2019
@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 10, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2019
@atiratree
Copy link
Member Author

/test e2e-aws-console-olm

@atiratree
Copy link
Member Author

@alecmerdler can you please take a look?

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 26, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 29, 2019
@atiratree
Copy link
Member Author

Things to consider:

  • we could generate the types just for development and not commit them into the repo. We would generate them each time yarn install or yarn dev is called. The generated files could be cached by comparing the commit hash of swagger.json.
    Main disadvantages:

    • our build depends on other repo (GH availability ) and location of certain file
    • we should lock our build to a release branch/tag of openshift/kubevirt to prevent building with potential breaking changes of the API. New versions still would have to be updated manually.
    • longer build time; +25s on my machine
  • currently, I am linting the generated files, but this step could be skipped by adding packages/kube-types into .eslintignore

@spadgett spadgett added this to the v4.3 milestone Aug 13, 2019
@atiratree
Copy link
Member Author

/retest

@mareklibra
Copy link
Contributor

In general, I like the idea of having comprehensive & updated types. Thanks for working on that. My concerns are just related to maintainability.

our build depends on other repo (GH availability ) and location of certain file

Exactly. I think it's better to keep the generated files or source for their generation in the repo. And commit changes wisely. In addition, it must be straightforward to do a build in an offline environment (which is already resolved for npm).

we should lock our build to a release branch/tag of openshift/kubevirt to prevent building with potential breaking changes of the API. New versions still would have to be updated manually.

IMO, this complexity out-weights benefits in the long-term.

currently, I am linting the generated files, but this step could be skipped by adding

I agree we do not need to care much about syntax in the generated code as long as the process for its creation is clear.

@atiratree
Copy link
Member Author

@alecmerdler can we proceed with this now for 4.3 ?

@spadgett spadgett removed this from the v4.3 milestone Dec 3, 2019
@openshift-bot
Copy link
Contributor

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci-robot openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 22, 2020
@atiratree
Copy link
Member Author

/lifecycle frozen

@openshift-ci-robot openshift-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 23, 2020
@openshift-ci-robot
Copy link
Contributor

@suomiy: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-gcp-console edc9f92 link /test e2e-gcp-console
ci/prow/e2e-gcp edc9f92 link /test e2e-gcp
ci/prow/analyze edc9f92 link /test analyze
ci/prow/kubevirt-plugin edc9f92 link /test kubevirt-plugin

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 17, 2025

@atiratree: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/kubevirt-plugin edc9f92 link /test kubevirt-plugin
ci/prow/ceph-storage-plugin edc9f92 link /test ceph-storage-plugin
ci/prow/images edc9f92 link true /test images
ci/prow/okd-scos-images edc9f92 link true /test okd-scos-images
ci/prow/e2e-gcp-console edc9f92 link true /test e2e-gcp-console
ci/prow/analyze edc9f92 link true /test analyze
ci/prow/frontend edc9f92 link true /test frontend
ci/prow/backend edc9f92 link true /test backend

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 17, 2025
@openshift-merge-robot
Copy link
Contributor

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@atiratree
Copy link
Member Author

stale

@atiratree atiratree closed this Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants