CONSOLE-4882: Support mailto: links in ConsoleLink href field #2592
Conversation
|
Pipeline controller notification For optional jobs, comment |
openshift-ci-robot
added
the
jira/valid-reference
|
@jhadvig: This pull request references CONSOLE-4882 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 story 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. |
WalkthroughThis pull request expands href field validation for ConsoleLink and ConsoleNotification APIs to accept both https URLs and mailto links. Changes include updating the validation regex pattern from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
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 Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
|
Hello @jhadvig! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
console/v1/types.go (1)
7-8: LGTM! The regex correctly adds mailto support.The pattern change from
^https://to^(https://|mailto:)correctly implements the stated goal, and the comment accurately describes the allowed formats.Optional enhancement: Consider adding more comprehensive validation for improved robustness:
- // href is the absolute URL for the link. Must use https:// for web URLs or mailto: for email links. - // +kubebuilder:validation:Pattern=`^(https://|mailto:)` + // href is the absolute URL for the link. Must use https:// for web URLs or mailto: for email links. + // +kubebuilder:validation:Pattern=`^(https://[^\s]+|mailto:[^\s]+)$`This would:
- Add an end anchor to validate the entire string
- Disallow whitespace in URLs/email addresses
- Provide stricter format validation
However, this is entirely optional—the current implementation maintains consistency with the original pattern and may be sufficient depending on how the href is validated/sanitized in the rendering layer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (9)
console/v1/tests/consolelinks.console.openshift.io/AAA_ungated.yaml(1 hunks)console/v1/types.go(1 hunks)console/v1/zz_generated.crd-manifests/00_consolelinks.crd.yaml(1 hunks)console/v1/zz_generated.crd-manifests/00_consolenotifications.crd.yaml(1 hunks)console/v1/zz_generated.featuregated-crd-manifests/consolelinks.console.openshift.io/AAA_ungated.yaml(1 hunks)console/v1/zz_generated.featuregated-crd-manifests/consolenotifications.console.openshift.io/AAA_ungated.yaml(1 hunks)console/v1/zz_generated.swagger_doc_generated.go(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
console/v1/zz_generated.crd-manifests/00_consolenotifications.crd.yamlconsole/v1/zz_generated.featuregated-crd-manifests/consolelinks.console.openshift.io/AAA_ungated.yamlconsole/v1/zz_generated.swagger_doc_generated.goconsole/v1/zz_generated.crd-manifests/00_consolelinks.crd.yamlopenapi/generated_openapi/zz_generated.openapi.goconsole/v1/tests/consolelinks.console.openshift.io/AAA_ungated.yamlconsole/v1/zz_generated.featuregated-crd-manifests/consolenotifications.console.openshift.io/AAA_ungated.yamlconsole/v1/types.goopenapi/openapi.json
🔇 Additional comments (10)
openapi/generated_openapi/zz_generated.openapi.go (2)
25676-25682: Consistent description update for Link.href field.The description matches the ConsoleLinkSpec.href field update, ensuring consistency across the OpenAPI schema.
24177-24183: Description is accurate and properly verified.The validation pattern in
console/v1/types.gohas been correctly updated to^(https://|mailto:), confirming alignment with the description in the generated OpenAPI schema. Both hunks properly document the expanded href support.console/v1/zz_generated.crd-manifests/00_consolenotifications.crd.yaml (1)
72-76: ConsoleNotification link href pattern and docs look consistentAllowing
https://andmailto:viapattern: ^(https://|mailto:)and updating the description keeps the validation behavior consistent with the rest of the API and just broadens the accepted schemes as intended.console/v1/tests/consolelinks.console.openshift.io/AAA_ungated.yaml (1)
21-35: New mailto ConsoleLink test is well-formedThe added test case cleanly exercises a
mailto:href with the minimal required fields and matching expected spec, providing good coverage for the new allowed scheme.console/v1/zz_generated.swagger_doc_generated.go (1)
15-18: Swagger doc for Link.href correctly reflects https + mailto supportThe updated description for
Link.hrefmatches the broadened validation (https for web URLs, mailto for email links) and keeps the distinction fromCLIDownloadLink(still https-only).console/v1/zz_generated.crd-manifests/00_consolelinks.crd.yaml (1)
83-86: ConsoleLink href schema aligns with new mailto supportThe href description and
pattern: ^(https://|mailto:)correctly broaden ConsoleLink to accept both https URLs and mailto links while preserving previous https behavior.console/v1/zz_generated.featuregated-crd-manifests/consolelinks.console.openshift.io/AAA_ungated.yaml (1)
82-85: Feature-gated ConsoleLink CRD stays in sync on href validationThe feature-gated CRD mirrors the updated href description and
^(https://|mailto:)pattern, keeping behavior consistent across gated and ungated manifests.console/v1/zz_generated.featuregated-crd-manifests/consolenotifications.console.openshift.io/AAA_ungated.yaml (1)
72-74: All changes verified and correctly sourced from types.go.The href validation pattern
^(https://|mailto:)in lines 72-74 correctly originates from the Link struct inconsole/v1/types.go, which defines the validation using the+kubebuilder:validation:Patterntag. The description accurately reflects the new support for mailto: links, and the generated manifest properly reflects the source definition. The regex pattern is well-formed and the file is correctly generated.openapi/openapi.json (2)
13306-13306: Pattern validation is correctly implemented in source code.The original concern is based on incomplete understanding. The href field pattern validation exists in the source file
console/v1/types.go(lines 7-8) via the kubebuilder marker+kubebuilder:validation:Pattern=^(https://|mailto:)`` on the Link struct. This marker is the authoritative source for validation enforcement and properly flows through to generated CRD manifests and the openapi.json descriptions. The PR changes to the openapi.json descriptions accurately document this validation behavior.Likely an incorrect or invalid review comment.
4782-4789: Regeneration of openapi.json is picking up pre-existing Go source documentation improvements, unrelated to the mailto: link feature.The git diff confirms your observation. The description expansions for
dnsRecordsType,idleConnectionTerminationPolicy, andhttpKeepAliveTimeoutare not new additions—they are pre-existing comments in the Go source files (config/v1/types_infrastructure.go and operator/v1/types_ingress.go). Whenopenapi.jsonis regenerated via thehack/update-openapi.shscript, it picks up these complete descriptions automatically. This PR's main change is supporting mailto: links in ConsoleLink, but the regenerated spec captures all pre-existing Go source documentation.These changes should be evaluated as part of the file regeneration process. If the openapi.json regeneration was performed intentionally as part of this PR, the expanded descriptions are an expected artifact. If it was unintended, consider reverting openapi.json to prevent scope creep, or split the regeneration into a separate maintenance PR.
|
@jhadvig: This pull request references CONSOLE-4882 which is a valid jira issue. 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. |
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Spoke with @jhadvig and this validation change is the only thing necessary to ensure that this works.
This is a very small and unlikely to break anything change.
/lgtm
|
[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
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/verified by @jhadvig |
openshift-ci-robot
added
the
verified
|
@jhadvig: This PR has been marked as verified by 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. |
|
@jhadvig: all tests passed! 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. |
5 participants
Tested the change on a live cluster and worked as desired.
/assign @spadgett @JoelSpeed @everettraven