DNM: test patch for inline review comments#2912
Conversation
Do not merge — throwaway PR for testing CI inline comment posting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Hello @theobarberbany! Some important instructions when contributing to openshift/api: |
openshift-ci
Bot
added
the
size/M
|
[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 |
theobarberbany
changed the title
![openshift-api-review-bot[bot]](https://avatars.githubusercontent.com/u/84759374?s=60&v=4){kind=small}
|
|
||
| // ExternalSecretStoreConfig defines the configuration for integration with an | ||
| // external secret store. | ||
| // +kubebuilder:validation:XValidation:rule="has(self.type) && self.type == 'Vault' ? has(self.vault) : !has(self.vault)",message="vault configuration is required when type is Vault, and forbidden otherwise" |
There was a problem hiding this comment.
Issue: Redundant has() guard on required field (CEL simplification)
The has(self.type) guard is unnecessary because type is a +required field — it is always present when XValidation runs. This makes the CEL expression more verbose than needed.
Current:
// +kubebuilder:validation:XValidation:rule="has(self.type) && self.type == 'Vault' ? has(self.vault) : !has(self.vault)",message="vault configuration is required when type is Vault, and forbidden otherwise"Suggested:
// +kubebuilder:validation:XValidation:rule="self.type == 'Vault' ? has(self.vault) : !has(self.vault)",message="vault configuration is required when type is Vault, and forbidden otherwise"Since type is +required, has(self.type) is always true. The guard adds no protection and makes the expression harder to read.
| // type specifies the type of external secret store to use. | ||
| // Currently supported values: | ||
| // - Vault | ||
| // +required |
There was a problem hiding this comment.
Issue: Enum value meaning not explained in field comment
The type field lists "Vault" as a supported value but does not explain what it means. Enum meanings must appear in the field's own comment (not just the type definition) since that is what users see in generated docs.
Current:
// type specifies the type of external secret store to use.
// Currently supported values:
// - Vault
// +required
Type ExternalSecretStoreType `json:"type"`Suggested:
// type specifies the type of external secret store to use.
// Currently supported values:
// - Vault: HashiCorp Vault is used as the external secret store.
// +required
Type ExternalSecretStoreType `json:"type"`Each enum value must be listed AND its meaning explained in the field's own comment using a descriptive pattern.
| // VaultSecretStoreConfig defines the configuration for HashiCorp Vault integration. | ||
| type VaultSecretStoreConfig struct { | ||
| // serverAddress specifies the address of the Vault server. | ||
| // Must be a valid URL starting with https://. |
There was a problem hiding this comment.
Issues: Documented constraint not enforced + MinLength/MaxLength not documented
Two problems with this field:
1) Documented constraint not enforced: The comment states "Must be a valid URL starting with https://" but there is no +kubebuilder:validation:Pattern marker to enforce this. The API will silently accept values that are not HTTPS URLs.
2) MinLength/MaxLength not documented: The field has MinLength=1 and MaxLength=512 markers but the comment does not document these length constraints.
Current:
// serverAddress specifies the address of the Vault server.
// Must be a valid URL starting with https://.
// +required
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=512
ServerAddress string `json:"serverAddress"`Suggested:
// serverAddress specifies the address of the Vault server.
// Must be a valid URL starting with https://.
// The address must be between 1 and 512 characters long.
// +required
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=512
// +kubebuilder:validation:Pattern=`^https://`
ServerAddress string `json:"serverAddress"`Either add a Pattern marker to enforce the HTTPS URL constraint, or remove the claim from the comment. All constraint markers must also be documented in the field's comment.
| ServerAddress string `json:"serverAddress"` | ||
|
|
||
| // transitKeyName specifies the name of the transit encryption key in Vault. | ||
| // The name must not exceed 253 characters. |
There was a problem hiding this comment.
Issues: Documented constraint not enforced + MinLength not documented
Two problems with this field:
1) Documented constraint not enforced: The comment states "The name must not exceed 253 characters" but there is no +kubebuilder:validation:MaxLength=253 marker to enforce this constraint.
2) MinLength not documented: The field has MinLength=1 but the comment does not mention that empty strings are rejected.
Current:
// transitKeyName specifies the name of the transit encryption key in Vault.
// The name must not exceed 253 characters.
// +required
// +kubebuilder:validation:MinLength=1
TransitKeyName string `json:"transitKeyName"`Suggested:
// transitKeyName specifies the name of the transit encryption key in Vault.
// The name must be at least 1 character and must not exceed 253 characters.
// +required
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=253
TransitKeyName string `json:"transitKeyName"`The claimed 253-character max must be enforced with a MaxLength marker, not just documented.
|
|
||
| // transitMountPath specifies the mount path for the transit secrets engine. | ||
| // +required | ||
| // +kubebuilder:validation:MinLength=1 |
There was a problem hiding this comment.
Issue: MinLength not documented
The field has MinLength=1 but the comment does not mention that the value must be at least 1 character (i.e., empty strings are rejected).
Current:
// transitMountPath specifies the mount path for the transit secrets engine.
// +required
// +kubebuilder:validation:MinLength=1
TransitMountPath string `json:"transitMountPath"`Suggested:
// transitMountPath specifies the mount path for the transit secrets engine.
// The path must be at least 1 character long.
// +required
// +kubebuilder:validation:MinLength=1
TransitMountPath string `json:"transitMountPath"`All constraint markers must be documented in the field's comment so users know the validation rules without reading the markers.
| TransitMountPath string `json:"transitMountPath"` | ||
|
|
||
| // namespace specifies the Vault namespace to use. | ||
| // +optional |
There was a problem hiding this comment.
Issues: Optional field missing omitted behavior + MinLength/MaxLength not documented
Two problems with this field:
1) Optional field does not explain omitted behavior: The namespace field is +optional but the comment does not explain what happens when the field is omitted.
2) MinLength/MaxLength not documented: The field has MinLength=1 and MaxLength=256 markers but the comment does not document these length constraints.
Current:
// namespace specifies the Vault namespace to use.
// +optional
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=256
Namespace string `json:"namespace,omitempty"`Suggested:
// namespace specifies the Vault namespace to use.
// When omitted, the root namespace is used.
// The namespace must be between 1 and 256 characters long.
// +optional
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=256
Namespace string `json:"namespace,omitempty"`Every optional field must explain the behavior when it is not provided. All constraint markers must also be documented in the field comment.
|
|
||
| // caCertificate contains the PEM-encoded CA certificate for TLS verification | ||
| // of the Vault server connection. | ||
| // When omitted, the system's trusted CA certificates are used. |
There was a problem hiding this comment.
Issue: MinLength and MaxLength not documented
The field has MinLength=1 and MaxLength=65536 markers but the comment does not document these length constraints.
Current:
// caCertificate contains the PEM-encoded CA certificate for TLS verification
// of the Vault server connection.
// When omitted, the system's trusted CA certificates are used.
// +optional
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=65536
CACertificate string `json:"caCertificate,omitempty"`Suggested:
// caCertificate contains the PEM-encoded CA certificate for TLS verification
// of the Vault server connection.
// When omitted, the system's trusted CA certificates are used.
// The certificate must be between 1 and 65536 characters long.
// +optional
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=65536
CACertificate string `json:"caCertificate,omitempty"`All constraint markers must be documented in the field comment so users understand validation without reading the markers.
API ReviewIssues were found and posted as inline comments on this PR.
|
1 participant
Do not merge — throwaway PR for testing CI inline comment posting.
Applies the kms-vault-config eval integration test patch to exercise the /api-review inline commenting flow.