Skip to content

Conversation

logonoff
Copy link
Member

No description provided.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Sep 16, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Sep 16, 2025

@logonoff: This pull request references CONSOLE-4775 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set.

In response to this:

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 16, 2025
Copy link
Contributor

openshift-ci bot commented Sep 16, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link
Contributor

openshift-ci bot commented Sep 16, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign jcantrill for approval. For more information see the Code Review Process.

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

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

1 similar comment
Copy link
Contributor

openshift-ci bot commented Sep 16, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign jcantrill for approval. For more information see the Code Review Process.

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

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

@logonoff logonoff force-pushed the CONSOLE-4775 branch 3 times, most recently from b2a5455 to 7ab9c50 Compare September 17, 2025 14:34
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

Thanks @logonoff, appreciate it. A few questions:

  • Is this a package we expect console plugin developers to use?
  • What types do we expose in the plugin SDK, and will these types replace those?
  • Will this introduce any breaking changes for plugins?

Comment on lines +213 to +222
2. Where should the type generation library be maintained? Should it be part of
`openshift/api`, or a separate repository?
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest we answer this question before merging the enhancement. My first inclination is to make this a separate repository.

Copy link
Member Author

Choose a reason for hiding this comment

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

There was some discussion around where the generation code should live when this proposal is implemented, so I've left this as an open question for now until we can answer it

Copy link
Member

Choose a reason for hiding this comment

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

I would say we just make it a separate repo in the openshift org. @logonoff @jhadvig Let me know if you guys disagree.

@logonoff
Copy link
Member Author

Thanks @logonoff, appreciate it. A few questions:

  • Is this a package we expect console plugin developers to use?

If a console plugin needs to read a resource which this types library supplies a type for, then they are free to use this package. Otherwise, I do not expect all console plugin developers to need this package. Future work could include adding extra sources of CRDs so that console plugins have a centralized place to get resource TypeScript types, but this is out of scope for the initial implementation

  • What types do we expose in the plugin SDK, and will these types replace those?

I believe we export K8sResourceCommon and its dependent types in the dynamic plugin SDK. We would be replacing K8sResourceCommon and ObjectMetadata with the new types, which are based off of the types in the dynamic plugin SDK. To reduce breakage I think we should be re-exporting these types in the console dynamic plugin SDK.

  • Will this introduce any breaking changes for plugins?

There are a few changes I've made to K8sResourceCommon to better align it with the expectations kubernetes has with these resources (e.g., I've typed managedFields in my version).

Moreover, since I switched from type to interface (per TypeScript's recommended heuristic), there are some cases with lodash's omit that don't work with interfaces but do work with types.

These changes do cause compilation to fail when I tried applying them in OpenShift console. However, I don't expect this to impact the backwards-compatibility for dynamic plugins as types get complied away at build time.

@logonoff logonoff marked this pull request as ready for review September 18, 2025 12:49
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 18, 2025
@spadgett
Copy link
Member

Future work could include adding extra sources of CRDs so that console plugins have a centralized place to get resource TypeScript types, but this is out of scope for the initial implementation

I agree this is out of scope and not a priority.

I believe we export K8sResourceCommon and its dependent types in the dynamic plugin SDK. We would be replacing K8sResourceCommon and ObjectMetadata with the new types, which are based off of the types in the dynamic plugin SDK. To reduce breakage I think we should be re-exporting these types in the console dynamic plugin SDK.

Thanks, agree on re-exporting.

There are a few changes I've made to K8sResourceCommon to better align it with the expectations kubernetes has with these resources (e.g., I've typed managedFields in my version).

Moreover, since I switched from type to interface (per TypeScript's recommended heuristic), there are some cases with lodash's omit that don't work with interfaces but do work with types.

These changes do cause compilation to fail when I tried applying them in OpenShift console. However, I don't expect this to impact the backwards-compatibility for dynamic plugins as types get complied away at build time.

So when plugins upgrade the version of the SDK package they use, they might need to fix type errors, but there are no runtime errors for plugins built with an older version, correct?

I'd add some of these details to the enhancement so it's clear what the impact is to plugins since we treat the SDK as a public API. Thanks @logonoff

@logonoff
Copy link
Member Author

So when plugins upgrade the version of the SDK package they use, they might need to fix type errors, but there are no runtime errors for plugins built with an older version, correct?

Right. Personally I also found that in openshift/console, the type errors were infrequent and easily fixable as well, so I don't see this as a big cause for concern

I'd add some of these details to the enhancement so it's clear what the impact is to plugins since we treat the SDK as a public API. Thanks @logonoff

Thanks, I've added it as a drawback

Copy link
Contributor

openshift-ci bot commented Sep 25, 2025

@logonoff: The following test 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/markdownlint 6443979 link true /test markdownlint

Full PR test history. Your PR dashboard.

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.

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

Labels

jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants