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 PR updates documentation strings and formatting across generated Go files, the OpenAPI outputs, and multiple Authentication CRD YAMLs. It also adds a new OpenAPI definition AcceptRisk and new properties: Update.acceptRisks, riskNames, and conditions. No existing public type or method signatures were changed beyond the added OpenAPI definitions and properties; remaining edits are descriptive or formatting-only. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
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. |
There was a problem hiding this comment.
Actionable comments posted: 1
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-CustomNoUpgrade.crd.yaml (1)
101-115: Align key path description with the validation regex.The description of allowed path characters doesn't mention
/, but the CEL regex at line 152 explicitly allows it:[A-Za-z0-9/\\-._~%!$&\'()*+;=:]+. Either add'/'to the documented character list (if intended) or remove it from the regex pattern.📌 Possible doc-only fix (if `/` is intended)
- The path portion of the key (string of characters after the '/') must not be empty and must consist of at least one alphanumeric character, percent-encoded octets, '-', '.', '_', '~', '!', '$', '&', ''', '(', ')', '*', '+', ',', ';', '=', and ':'. + The path portion of the key (string of characters after the '/') must not be empty and must consist of at least one alphanumeric character, percent-encoded octets, '/', '-', '.', '_', '~', '!', '$', '&', ''', '(', ')', '*', '+', ',', ';', '=', and ':'.Also applies to: line 152
🤖 Fix all issues with AI agents
In
`@payload-manifests/crds/0000_10_config-operator_01_authentications-Default.crd.yaml`:
- Around line 196-201: Fix the typo in the description for the `prefix` field
where the example contains a double space between `"b" and "c"`; update the
text inside the `description` block for the `prefix` field (the multiline string
under `description:`) to use a single space so it reads `"b" and "c"`. Ensure
the rest of the wording and punctuation remain unchanged.
payload-manifests/crds/0000_10_config-operator_01_authentications-Default.crd.yaml
Outdated
Show resolved
Hide resolved
|
Hey @everettraven - I have a few examples here but can get more if needed. spec.oidcProviders.claimMappingsBefore (master)After (current branch)spec.oidcProviders.claimMappings.groupsBefore (master)After (current branch)spec.oidcProviders.claimMappings.usernameBefore (master)After (current branch) |
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)
436-504: Minor grammar nit in extraScopes description.
Line 503: “claim mappings that requires” → “claim mappings that require.”✅ Proposed fix
- This is useful if you have configured claim mappings that requires specific scopes to be requested beyond the standard OIDC scopes. + This is useful if you have configured claim mappings that require specific scopes to be requested beyond the standard OIDC scopes.
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Thanks for the before/after comparisons!
Aside from a couple more sentence breaks, this looks good to me.
|
Hoping I caught those pesky comments ! :D Thanks again @everettraven for the reviews. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@payload-manifests/crds/0000_10_config-operator_01_authentications-Default.crd.yaml`:
- Around line 196-200: The example text for the `prefix` description contains a
grammatical error ("an array of string") — update the sentence in the `prefix`
field documentation so it reads "an array of strings" (referencing the `prefix`
and `claim` example that maps JWT claim values to cluster identity attributes)
to correct the pluralization in the example description.
| prefix is an optional field that configures the prefix that will be applied to the cluster identity attribute during the process of mapping JWT claims to cluster identity attributes. | ||
| When omitted (""), no prefix is applied to the cluster identity attribute. | ||
| Example: if `prefix` is set to "myoidc:" and the `claim` in JWT contains | ||
| an array of strings "a", "b" and "c", the mapping will result in an | ||
| array of string "myoidc:a", "myoidc:b" and "myoidc:c". | ||
| Example: if `prefix` is set to "myoidc:" and the `claim` in JWT contains an array of strings "a", "b" and "c", the mapping will result in an array of string "myoidc:a", "myoidc:b" and "myoidc:c". |
There was a problem hiding this comment.
Grammatical error: "an array of string" should be "an array of strings".
Line 200 contains a minor grammatical error in the example text.
Suggested fix
- Example: if `prefix` is set to "myoidc:" and the `claim` in JWT contains an array of strings "a", "b" and "c", the mapping will result in an array of string "myoidc:a", "myoidc:b" and "myoidc:c".
+ Example: if `prefix` is set to "myoidc:" and the `claim` in JWT contains an array of strings "a", "b" and "c", the mapping will result in an array of strings "myoidc:a", "myoidc:b" and "myoidc:c".📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| prefix is an optional field that configures the prefix that will be applied to the cluster identity attribute during the process of mapping JWT claims to cluster identity attributes. | |
| When omitted (""), no prefix is applied to the cluster identity attribute. | |
| Example: if `prefix` is set to "myoidc:" and the `claim` in JWT contains | |
| an array of strings "a", "b" and "c", the mapping will result in an | |
| array of string "myoidc:a", "myoidc:b" and "myoidc:c". | |
| Example: if `prefix` is set to "myoidc:" and the `claim` in JWT contains an array of strings "a", "b" and "c", the mapping will result in an array of string "myoidc:a", "myoidc:b" and "myoidc:c". | |
| prefix is an optional field that configures the prefix that will be applied to the cluster identity attribute during the process of mapping JWT claims to cluster identity attributes. | |
| When omitted (""), no prefix is applied to the cluster identity attribute. | |
| Example: if `prefix` is set to "myoidc:" and the `claim` in JWT contains an array of strings "a", "b" and "c", the mapping will result in an array of strings "myoidc:a", "myoidc:b" and "myoidc:c". |
🤖 Prompt for AI Agents
In
`@payload-manifests/crds/0000_10_config-operator_01_authentications-Default.crd.yaml`
around lines 196 - 200, The example text for the `prefix` description contains a
grammatical error ("an array of string") — update the sentence in the `prefix`
field documentation so it reads "an array of strings" (referencing the `prefix`
and `claim` example that maps JWT claim values to cluster identity attributes)
to correct the pluralization in the example description.
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/retest |
|
@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. |
|
@xingxingxia could you take a look and verify when you have the time? :) |
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.