Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve ClusterServiceVersion schema #168

Merged
merged 1 commit into from
Mar 28, 2023
Merged

Improve ClusterServiceVersion schema #168

merged 1 commit into from
Mar 28, 2023

Conversation

TheRealJon
Copy link
Contributor

@TheRealJon TheRealJon commented Nov 2, 2021 •
edited

Fixes #167
Add descriptions for kind, name, and version properties of APIResourceReference type as well as DisplayName, Description, Keywords, Maintainers, Provider, Links, and Icon properties of ClusterServiceVersionSpec.

@TheRealJon
Copy link
Contributor Author

I realize now after opening this that the manifests are generated files. First time contributing to an API repo like this, so I'll research a bit more and update this.

@TheRealJon
Copy link
Contributor Author

/hold

@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 Nov 2, 2021
@TheRealJon TheRealJon changed the title Improve ClusterServiceVersion schema [WIP] Improve ClusterServiceVersion schema Nov 2, 2021
@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 Nov 2, 2021
@TheRealJon
Copy link
Contributor Author

/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 Nov 2, 2021
@TheRealJon TheRealJon changed the title [WIP] Improve ClusterServiceVersion schema Improve ClusterServiceVersion schema Nov 2, 2021
@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 Nov 2, 2021
camilamacedo86
camilamacedo86 reviewed Jan 15, 2022
View reviewed changes
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2022
@camilamacedo86
Copy link
Contributor

/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 15, 2022
@openshift-ci openshift-ci bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 28, 2022
@TheRealJon
Copy link
Contributor Author

@camilamacedo86 I rebased this and added a few more descriptions to the CSV API so it will need another review/lgtm

dinhxuanvu
dinhxuanvu approved these changes Apr 14, 2022
View reviewed changes
Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci
Copy link

openshift-ci bot commented Apr 14, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu, TheRealJon

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

The pull request process is described 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 14, 2022
@jhadvig
Copy link

jhadvig commented May 11, 2022

@dinhxuanvu could you please retag the PR ? thanks ! :)

@camilamacedo86
Copy link
Contributor

It seems blocked by the resolution in : operator-framework/operator-lifecycle-manager#2767
The concern is it increase the size of the CRDs and API only allows 1MB.

exdx
exdx reviewed May 11, 2022
View reviewed changes
Copy link
Member

@exdx exdx left a comment

Choose a reason for hiding this comment

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

This looks like a helpful PR, and does improve the descriptions on the CSV API, but there is an issue with the size of the CRD when applied via kubectl apply as the last-applied-configuration annotation becomes too large for the server to accept. Increasing the size of the openapi schema will further complicate this issue.

The long term solution is to limit the depth of the codegen generated for the openapi spec, to prevent this issue from occurring. That being said I don't know whether we should prevent this PR from going in based on the issue, until the longer term fix is prepared.

@TheRealJon
Copy link
Contributor Author

This looks like a helpful PR, and does improve the descriptions on the CSV API...

Yes, and I would go further to say that these changes are a necessary improvement. The lack of documentation on these APIs has led to confusion about their usage and, a few bugs have been reported. Better docs would have saved a lot of time and effort spent for each bug that has been opened for this.

...but there is an issue with the size of the CRD when applied via kubectl apply as the last-applied-configuration annotation becomes too large for the server to accept. Increasing the size of the openapi schema will further complicate this issue

If I'm understanding correctly, the annotation size error has already been encountered and a workaround has been implemented. If that's the case, does this PR pose any further risk?

The long term solution is to limit the depth of the codegen generated for the openapi spec, to prevent this issue from occurring. That being said I don't know whether we should prevent this PR from going in based on the issue, until the longer term fix is prepared.

Would limiting the codegen depth prevent deeper APIs from having docs generated?

@TheRealJon
Copy link
Contributor Author

TheRealJon commented Jun 9, 2022 •
edited

@msugakov spec.customresourcedefinitions.owned.[*].resources is an API used only by the web console. We use this to list resources that are managed by the operator in the topology view.

@TheRealJon
Copy link
Contributor Author

This PR is blocked by operator-framework/operator-lifecycle-manager#2778

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 23, 2023
@TheRealJon
Copy link
Contributor Author

@camilamacedo86 @exdx @porridge I've rebased this PR. Could we revisit getting this merged?

@perdasilva
Copy link
Contributor

@TheRealJon you might need to do a make manifests or make verify to fix the verify stage

@TheRealJon
Copy link
Contributor Author

@perdasilva I've updated it, hopefully, this should pass now.

@grokspawn
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 24, 2023
@grokspawn
Copy link
Contributor

grokspawn commented Mar 24, 2023 •
edited

/hold to see if we can catch a review (lgtm) from one of the historical participants

@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 Mar 24, 2023
@tlwu2013
Copy link

+1, lgtm too, thanks for your PR @TheRealJon !

@TheRealJon
Copy link
Contributor Author

TheRealJon commented Mar 24, 2023 •
edited

@dinhxuanvu I brought this up in the k8s Slack #olm-dev channel to loop @grokspawn in. If both of you are in agreement, I think this is ready to merge.

@TheRealJon
Improve ClusterServiceVersion schema.
91c94cf 
Add descriptions for kind, name, and version properties of APIResourceReference type as well as DisplayName, Description, Keywords, Maintainers, Provider, Links, and Icon properties of ClusterServiceVersionSpec.
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2023
@TheRealJon
Copy link
Contributor Author

@joelanford @grokspawn I've updated the wording to be more clear. PTAL!

@grokspawn
Copy link
Contributor

/hold cancel

releasing this since it looks like we had some good dialog and reached a consensus.

@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 Mar 28, 2023
@joelanford
Copy link
Member

/lgtm

Thanks!

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 28, 2023
@openshift-merge-robot openshift-merge-robot merged commit 36145b1 into operator-framework:master Mar 28, 2023
@TheRealJon TheRealJon deleted the issue-167 branch March 29, 2023 16:08
This was referenced Apr 25, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@porridge porridge porridge left review comments

@joelanford joelanford joelanford left review comments

@camilamacedo86 camilamacedo86 camilamacedo86 left review comments

@exdx exdx exdx left review comments

@dinhxuanvu dinhxuanvu dinhxuanvu approved these changes

@anik120 anik120 Awaiting requested review from anik120

@ankitathomas ankitathomas Awaiting requested review from ankitathomas

Assignees

@joelanford joelanford

@camilamacedo86 camilamacedo86

@grokspawn grokspawn

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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve API documentation for CSV and PackageManifest