OCPBUGS-56851: make multiline comments for oidc one line for better crd gen and better oc explain description format
#2655
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Hello @ehearne-redhat! Some important instructions when contributing to openshift/api: |
openshift-ci-robot
added
jira/severity-low
|
@ehearne-redhat: This pull request references Jira Issue OCPBUGS-56851, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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. |
📝 WalkthroughWalkthroughThe pull request updates documentation strings and reflows comment text across generated Go files, OpenAPI outputs, and multiple CRD YAMLs. It adds a new OpenAPI definition AcceptRisk and introduces Update.acceptRisks, riskNames, and conditions fields in the OpenAPI schema. One CRD (OKD) changes oidcProviders.username.prefix from a string to an object with a required prefixString. Aside from these schema additions and the single CRD structural change, all other edits are textual and do not alter existing public type signatures or validation rules. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
openshift-ci
bot
added
the
size/L
|
@ShazaAldawamneh could you review when you have the chance? :) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@config/v1/types_authentication.go`:
- Around line 293-296: Update the comment that currently says "must differ from
the value of `url`" to reference `issuerURL` instead so it matches the JSON
field name and existing validation message; locate the block describing
discoveryURL (identifier: discoveryURL) and replace `url` with `issuerURL` to
keep the docstring consistent with issuerURL/validation text.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
config/v1/types_authentication.go
🧰 Additional context used
🧬 Code graph analysis (1)
config/v1/types_authentication.go (1)
config/v1/zz_generated.swagger_doc_generated.go (8)
TokenIssuer(525-527)TokenClaimMappings(484-486)TokenClaimValidationRule(514-516)ConfigMapNameReference(86-88)UsernameClaimMapping(554-556)PrefixedClaimMapping(464-466)TokenClaimOrExpressionMapping(494-496)SecretNameReference(230-232)
🔇 Additional comments (8)
config/v1/types_authentication.go (8)
201-251: LGTM!The comment reformatting for the
OIDCProviderstruct fields successfully consolidates the multiline descriptions into single-line comments. This will improveoc explainoutput formatting as intended.
310-350: LGTM!The reformatted comments for
TokenClaimMappingsfields (username, groups, uid, extra) are clear and maintain the original semantic meaning while achieving the single-line format goal.
468-524: LGTM!The
OIDCClientConfigstruct field comments are well-reformatted. The descriptions forcomponentName,componentNamespace,clientID,clientSecret, andextraScopesremain clear and informative.
526-582: LGTM!The
OIDCClientStatusstruct comments are properly reformatted to single-line style while maintaining clarity.
584-610: LGTM!The
OIDCClientReferencestruct field comments are correctly reformatted.
612-661: LGTM!The
UsernameClaimMappingandUsernamePrefixPolicydocumentation changes properly consolidate the multiline descriptions while preserving the detailed examples and behavior documentation for prefix handling.
731-753: LGTM!The
TokenClaimValidationRulestruct comments are properly reformatted. The descriptions for thetypeandrequiredClaimfields remain clear.
796-816: LGTM!The
TokenUserValidationRulestruct comments are properly reformatted to single-line format.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
ehearne-redhat
force-pushed
the
fix-crd-godoc-gen-oidc
branch
from
28a3711 to
fcb97da
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
openshift-ci
bot
added
size/XXL
|
/assign @everettraven |
|
/test verify |
|
PR-Agent: could not fine a component named |
|
Verify should only fail on this repo if you have out of date generation, I can see in the logs it's showing a diff so you'll need to investigate this |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
payload-manifests/crds/0000_10_config-operator_01_authentications-OKD.crd.yaml (1)
375-386: Update description to match enum: remove CEL references from OKD variant.The description mentions "Allowed values are 'RequiredClaim' and 'CEL'" but the enum constraint only permits
RequiredClaim. This inconsistency exists because CEL support is intentionally excluded from the OKD feature set (unlike CustomNoUpgrade, DevPreviewNoUpgrade, and TechPreviewNoUpgrade variants which do include it). Update the description to remove CEL references and only documentRequiredClaimfor accuracy.
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Overall, this looks good to me. I've just got a few comments related to using sentences as our natural breakpoints for balancing comment and generated documentation readability.
Do we have a visual representation of the difference that we can show this change makes?
|
Hey @everettraven - I spun up a kind cluster and applied the old and new CRD OLD ehearne-mac:api ehearne$ kubectl explain authentication.spec.oidcProviders.claimMappings.extra
GROUP: config.openshift.io
KIND: Authentication
VERSION: v1
FIELD: extra <[]Object>
DESCRIPTION:
extra is an optional field for configuring the mappings
used to construct the extra attribute for the cluster identity.
When omitted, no extra attributes will be present on the cluster identity.
key values for extra mappings must be unique.
A maximum of 32 extra attribute mappings may be provided.
ExtraMapping allows specifying a key and CEL expression
to evaluate the keys' value. It is used to create additional
mappings and attributes added to a cluster identity from
a provided authentication token.
...NEW: ehearne-mac:api ehearne$ kubectl explain authentication.spec.oidcProviders.claimMappings.extra
GROUP: config.openshift.io
KIND: Authentication
VERSION: v1
FIELD: extra <[]Object>
DESCRIPTION:
extra is an optional field for configuring the mappings used to construct
the extra attribute for the cluster identity.
When omitted, no extra attributes will be present on the cluster identity.
key values for extra mappings must be unique.
A maximum of 32 extra attribute mappings may be provided.
ExtraMapping allows specifying a key and CEL expression
to evaluate the keys' value. It is used to create additional
mappings and attributes added to a cluster identity from
a provided authentication token.
... |
|
@ehearne-redhat Thanks for that! Do we have a list of what looked to be the worst offenders that we can verify look better with these changes? |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
payload-manifests/crds/0000_10_config-operator_01_authentications-OKD.crd.yaml (1)
377-387: Fix doc/schema mismatch forclaimValidationRules.typein OKD variant.The description mentions "Allowed values are 'RequiredClaim' and 'CEL'", but the enum only lists
RequiredClaimand thecelfield is absent from the schema. This mismatch does not exist in other variants (TechPreviewNoUpgrade, CustomNoUpgrade, DevPreviewNoUpgrade) which correctly include CEL in both the enum and schema. The OKD variant should have its description updated to remove CEL references.Suggested fix
- Allowed values are "RequiredClaim" and "CEL". - - When set to 'RequiredClaim', the Kubernetes API server will be configured to validate that the incoming JWT contains the required claim and that its value matches the required value. - - When set to 'CEL', the Kubernetes API server will be configured to validate the incoming JWT against the configured CEL expression. + Allowed values are "RequiredClaim". + + When set to 'RequiredClaim', the Kubernetes API server will be configured to validate that the incoming JWT contains the required claim and that its value matches the required value.
|
Hey @everettraven - I don't have a specific list to hand. However, I believe the ones that were the big offenders were What I'll do is, I'll compile a list of all these changes old vs new, and pick out a few which demonstrate this change best. |
|
@ehearne-redhat: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
4 participants
This change specifically targets OIDC-specific comments to ensure
oc explain, based on CRD generation, will render theDescriptionfield in an optimal format i.e. no disjointed lines.