SPLAT-2167: Added dedicated host support for AWS #374
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@vr4manta: This pull request references SPLAT-2167 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. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Go module bumps e2e/go.mod, go.mod, hack/tools/go.mod, manifests-gen/go.mod |
Bump many dependencies: Kubernetes k8s.io/* → v0.34.1, golang.org/x/* updates, sigs.k8s.io/yaml v1.6.0, fsnotify v1.9.0, fxamacker/cbor v2.9.0, cel-go and related indirects. |
AWS HostAffinity conversion pkg/conversion/capi2mapi/aws.go, pkg/conversion/mapi2capi/aws.go |
Add conversion mapping CAPA HostAffinity ("default"/"host") ⇄ MAPI HostPlacement (AnyAvailable/DedicatedHost); validate HostID format; new helpers and error constants; populate HostAffinity/HostID in conversions. |
AWS tests & fuzzers pkg/conversion/capi2mapi/aws_fuzz_test.go, pkg/conversion/mapi2capi/aws_fuzz_test.go, pkg/conversion/capi2mapi/aws_test.go, pkg/conversion/mapi2capi/aws_test.go |
Extend fuzzers and table tests for host placement variants; add fuzz functions and test entries; adjust many expected error/warning string formats to more compact/JSON-like representations. |
Other conversion test updates pkg/conversion/*_test.go (machine_test.go, openstack_test.go, powervs_test.go, mapi2capi/machine_test.go, mapi2capi/powervs_test.go) |
Update expected error/warning message representations (durations, taints, resource refs) to new compact/JSON-like forms; no logic changes. |
AWS IAM manifest manifests/0000_30_cluster-api_01_credentials-request.yaml |
Add ec2:DescribeInstanceTypes to AWS provider statement entries. |
fsnotify refactor & ergonomics vendor/github.com/fsnotify/fsnotify/* (backend_.go, shared.go, internal/, fsnotify.go, CHANGELOG/README/.cirrus.yml/staticcheck.conf) |
Replace per-backend done channels with a shared lifecycle (shared), centralize event/error sending, improve error wrapping and symlink/not-exist handling, add defaultBufferSize, rename SyscallEACCES/UnixEACCES → ErrSyscallEACCES/ErrUnixEACCES, and update docs/CI/changelog. |
fxamacker/cbor v2.9.0 vendor/github.com/fxamacker/cbor/v2/* (encode.go, decode.go, stream.go, cache.go, bytestring.go, simplevalue.go, structfields.go, tag.go, omitzero_*.go, README.md, common.go, etc.) |
Major API changes: replace many interface{} usages with any (Marshal/Unmarshal/Decoder/Encoder), add TextMarshaler/TextUnmarshaler modes and Transcoder interface, UnmarshalCBOR well-formedness wrappers, omitzero support, reflect.Pointer updates, cache/isZero plumbing, and Go 1.24 compat files. |
CEL-Go vendored upgrades & features vendor/github.com/google/cel-go/** (cel/, common/, ext/, interpreter/, checker/, templates/, BUILD.bazel, etc.) |
Large vendored changes: config model (common/env) and FromConfig, Activation/PartialActivation aliases and NewActivation, ToConfig/ExtensionOptionFactory, documentation AST and docs APIs, overload docs/examples, string.format v2 and formatting_v2, list cost tracking, new extensions (regex, math.sqrt), ObservableInterpretable, PlannerOption/StatefulObserver/EvalState, and many public helper APIs. |
Miscellaneous vendor / build vendor/cel.dev/expr/MODULE.bazel, various vendor BUILD files and added sources |
Bazel and build updates: add new sources, templates, tests, and deps for newly vendored functionality (templates, prompt, formatting_v2, regex, format.go, etc.). |
Sequence Diagram(s)
sequenceDiagram
participant C as Converter
participant CAPA as CAPI AWSMachine
participant MAPI as MAPI ProviderSpec
Note over C,CAPA: HostAffinity → HostPlacement mapping
CAPA->>C: AWSMachine spec (HostAffinity, HostID)
C->>C: validate HostAffinity/HostID (regex for dedicated host ID)
alt HostAffinity == "host"
C->>MAPI: HostPlacement = DedicatedHost{ID: HostID}
else HostAffinity == "default" or nil
C->>MAPI: HostPlacement = AnyAvailable
else invalid
C->>C: append errUnsupportedHostAffinityType / errHostIDRequred
end
C->>Caller: return providerSpec or errors
sequenceDiagram
participant U as User
participant W as fsnotify.Watcher
participant S as shared
Note over W,S: shared lifecycle for backends
U->>W: newBackend(ev, errs)
W->>S: shared = newShared(ev, errs)
W->>W: start readEvents goroutine
loop filesystem events
W->>W: handleEvent(raw)
W->>S: shared.sendEvent(Event{...})
S->>U: deliver event on Events channel
end
U->>W: Close()
W->>S: shared.close()
S->>W: signal readEvents to drain/stop
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~60 minutes
Focus areas for review:
- CEL-Go interpreter/planner/stateful observer and ObservableInterpretable public API changes (vendor/github.com/google/cel-go/interpreter, interpretable).
- fsnotify backend refactor across platforms (fen, inotify, kqueue, windows): shared lifecycle, Close/drain semantics, error wrapping and symlink/not-exist handling.
- fxamacker/cbor public API changes (interface{} → any), new Transcoder/TextMarshaler/TextUnmarshaler options, and well-formedness/UnmarshalCBOR helpers.
- AWS HostAffinity/HostID conversion logic and associated tests/fuzzers (pkg/conversion/*).
- common/env FromConfig, ExtensionOptionFactory, templates and newly added vendored extensions (regex, formatting_v2) for integration and build correctness.
Poem
🐇 I hopped through modules, small and spry,
HostAffinity tucked beneath the sky.
fsnotify found a shared, steady beat,
CBOR and CEL learned new ways to meet.
A rabbit's cheer — merge, test, and fly!
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 68.33% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The PR title clearly summarizes the primary feature: AWS dedicated host support. It's concise, specific, and directly reflects the main change in the changeset. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
Comment @coderabbitai help to get the list of available commands and usage tips.
openshift-merge-robot
added
the
needs-rebase
|
@vr4manta: This pull request references SPLAT-2167 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. |
openshift-merge-robot
removed
the
needs-rebase
vr4manta
force-pushed
the
SPLAT-2167
branch
2 times, most recently
from
6355141 to
7ba97c3
Compare
|
@vr4manta: This pull request references SPLAT-2167 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. |
vr4manta
force-pushed
the
SPLAT-2167
branch
2 times, most recently
from
dc53d82 to
e5cbce2
Compare
|
/test all |
|
/test all |
vr4manta
force-pushed
the
SPLAT-2167
branch
2 times, most recently
from
ed41da7 to
320eac0
Compare
|
@vr4manta: This pull request references SPLAT-2167 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. |
| - ec2:DescribeDhcpOptions | ||
| - ec2:DescribeImages | ||
| - ec2:DescribeInstances | ||
| - ec2:DescribeInstanceTypes |
There was a problem hiding this comment.
I believe it would be nice to add this change to a new commit referencing the reason of adding it (upstream PR, jira feature, etc).
this would be used to document in the future and/or in managed services to eventually update the managed IAM.
There was a problem hiding this comment.
Sure! There is a warning message that is caused by this missing. I am not 100% sure this feature introduced it, but I am happy to put into a separate commit.
| mapaProviderConfig.HostPlacement = &mapiv1beta1.HostPlacement{ | ||
| Affinity: ptr.To(mapiv1beta1.HostAffinityDedicatedHost), | ||
| DedicatedHost: &mapiv1beta1.DedicatedHost{ | ||
| ID: *m.awsMachine.Spec.HostID, |
There was a problem hiding this comment.
do we need to check HostID isnt nil to be safe or is there already a previous validation of this scenario?
There was a problem hiding this comment.
good question. lots of these CRs have checks in their respective webhooks / CRDs. I can add a check here as well. I was on the fence about it.
|
@vr4manta: This pull request references SPLAT-2167 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. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@vr4manta: This pull request references SPLAT-2167 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. |
![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
📜 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 (12)
manifests/0000_30_cluster-api_01_credentials-request.yaml(1 hunks)pkg/conversion/capi2mapi/aws.go(2 hunks)pkg/conversion/capi2mapi/aws_fuzz_test.go(3 hunks)pkg/conversion/capi2mapi/aws_test.go(6 hunks)pkg/conversion/capi2mapi/machine_test.go(1 hunks)pkg/conversion/capi2mapi/openstack_test.go(6 hunks)pkg/conversion/capi2mapi/powervs_test.go(3 hunks)pkg/conversion/mapi2capi/aws.go(2 hunks)pkg/conversion/mapi2capi/aws_fuzz_test.go(3 hunks)pkg/conversion/mapi2capi/aws_test.go(8 hunks)pkg/conversion/mapi2capi/machine_test.go(1 hunks)pkg/conversion/mapi2capi/powervs_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/conversion/mapi2capi/powervs_test.go
- manifests/0000_30_cluster-api_01_credentials-request.yaml
- pkg/conversion/mapi2capi/machine_test.go
- pkg/conversion/mapi2capi/aws.go
- pkg/conversion/capi2mapi/machine_test.go
- pkg/conversion/capi2mapi/openstack_test.go
- pkg/conversion/capi2mapi/aws_fuzz_test.go
🔇 Additional comments (8)
pkg/conversion/mapi2capi/aws_fuzz_test.go (2)
167-180: LGTM! Well-structured fuzzing for HostPlacement.The fuzzing function correctly exercises both valid HostAffinity variants (AnyAvailable and DedicatedHost with a valid host ID pattern), providing good coverage for the new HostPlacement feature.
140-140: Good - clearing CPUOptions for fuzzing.This aligns with the pattern of clearing unsupported fields in the provider config during fuzzing.
pkg/conversion/capi2mapi/powervs_test.go (1)
100-100: LGTM! Test expectations updated for JSON-like error formatting.The error message updates reflect a consistent pattern of representing nested objects in JSON-like format rather than Go struct notation, aligning with Kubernetes API conventions.
Also applies to: 125-125, 150-150
pkg/conversion/capi2mapi/aws.go (1)
172-194: HostAffinity conversion logic is sound.The implementation correctly handles the three cases (host with HostID, default, and unsupported values) and properly validates that HostID is present when needed. The nil check on line 176 addresses the concern from the past review comment.
pkg/conversion/mapi2capi/aws_test.go (2)
96-121: Good test coverage for HostPlacement scenarios.The test entries appropriately cover both valid HostPlacement configurations (AnyAvailable and DedicatedHost). The test structure integrates well with the existing table-driven test approach.
132-132: LGTM! Error message formatting updated consistently.The JSON-like representation of nested objects in error messages aligns with Kubernetes API conventions and matches the formatting changes across other test files in this PR.
Also applies to: 211-211, 246-246, 258-258, 348-348, 394-394
pkg/conversion/capi2mapi/aws_test.go (2)
72-98: Comprehensive test coverage for HostAffinity conversion.The three test entries thoroughly exercise the HostAffinity conversion logic: valid "default" affinity, valid "host" affinity with HostID, and unsupported affinity value with error handling. Good coverage of both positive and negative scenarios.
138-138: LGTM! Consistent error formatting across test expectations.The JSON-like representation of complex objects in error messages maintains consistency with the broader error formatting improvements throughout this PR.
Also applies to: 147-147, 171-171, 179-179, 190-190, 203-203
|
@vr4manta: This pull request references SPLAT-2167 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. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
vendor/github.com/google/cel-go/cel/BUILD.bazel (1)
1-98: Incomplete vendoring of google/cel-go v0.26.0: missing test files and testdata directory will cause build failures.The BUILD.bazel file references files and directories that do not exist in the vendored google/cel-go:
- Line 70:
prompt_test.golisted in test srcs — file not found invendor/github.com/google/cel-go/cel/- Lines 79-81: embedsrcs references
//cel/testdata:prompts—testdata/directory does not existThe vendored library version v0.26.0 is appropriate (no CVEs), but the vendor is incomplete. These missing files will cause Bazel build failures when the test target attempts to compile.
Re-vendor google/cel-go v0.26.0 to include the missing prompt_test.go file and testdata directory.
vendor/github.com/google/cel-go/ext/formatting.go (1)
313-341: Fix%eprecision format string instringFormatter.ScientificThe
Scientificmethod builds the format string asfmt.Sprintf("%%%de", *precision), which produces"%3e"(field width 3) instead of"%.3e"(precision 3). TheFixedmethod at line 303 correctly usesfmt.Sprintf("%%.%df", *precision).Change line 339 to:
- fmtStr := fmt.Sprintf("%%%de", *precision) + fmtStr := fmt.Sprintf("%%.%de", *precision)This ensures the precision parameter is correctly applied to scientific notation output.
vendor/github.com/fsnotify/fsnotify/backend_windows.go (1)
79-89:sendErrorcan steal the Close() handshake and deadlock shutdownWhen
Close()writes its reply channel intow.doneand wakes the IO thread, there may still be pending completion events. For those, the IO thread runs the normal path (wherewatch != nil) and can callsendError. BecausesendErrorcurrently does:select { case <-w.done: return false case w.Errors <- err: return true }it may consume the handshake value from
w.donebeforereadEvents()’swatch == nilbranch sees it. In that case:
- The
watch == nilbranch’sselectnever receivesch, so the cleanup/close logic at Lines 496–517 is never executed.Close()stays blocked on<-chforever.- The IO thread keeps running and resources are leaked.
To preserve the handshake semantics and avoid this deadlock,
sendErrorshould mirrorsendEventby re‑queueing the channel and treating its presence as “watcher is closing”, without discarding it:func (w *readDirChangesW) sendError(err error) bool { if err == nil { return true } - select { - case <-w.done: - return false - case w.Errors <- err: - return true - } + select { + case ch := <-w.done: + // Preserve the Close() handshake message for readEvents() and + // signal to callers that the watcher is closing. + w.done <- ch + return false + case w.Errors <- err: + return true + } }This keeps the Close handshake intact while still allowing callers of
sendErrorto detect that the watcher is shutting down.Also applies to: 495-517
vendor/github.com/fsnotify/fsnotify/backend_inotify.go (1)
82-110: Recursive inotify removal can over‑remove watches due to plainHasPrefixIn
watches.removePath, the recursive removal case uses:for p, rwd := range w.path { if strings.HasPrefix(p, path) { delete(w.path, p) delete(w.wd, rwd) wds = append(wds, rwd) } }This treats any path that merely shares a string prefix as being “under” the recursive root. For example, removing
/tmp/foo/...will also match/tmp/foobar, so a recursive watch on/tmp/foocan inadvertently tear down watches on/tmp/foobar.Consider tightening the condition to be path‑separator aware so only true descendants are removed:
- for p, rwd := range w.path { - if strings.HasPrefix(p, path) { + for p, rwd := range w.path { + if p == path || strings.HasPrefix(p, path+string(os.PathSeparator)) { delete(w.path, p) delete(w.wd, rwd) wds = append(wds, rwd) } }This preserves the intended recursive behavior without affecting unrelated paths that just happen to share a prefix.
vendor/github.com/fsnotify/fsnotify/backend_kqueue.go (1)
608-638:dirChangestops scanning the directory on benign per‑file errorsIn
dirChange, when iterating directory entries:fi, err := f.Info() if err != nil { if errors.Is(err, os.ErrNotExist) { return nil } return fmt.Errorf("fsnotify.dirChange: %w", err) } ... err = w.sendCreateIfNew(filepath.Join(dir, fi.Name()), fi) if err != nil { // Don't need to send an error if this file isn't readable. if errors.Is(err, unix.EACCES) || errors.Is(err, unix.EPERM) || errors.Is(err, os.ErrNotExist) { return nil } return fmt.Errorf("fsnotify.dirChange: %w", err) }For races where a single file disappears between
ReadDirandInfo(), or where a specific file is unreadable, this code returnsniland stops processing the rest of the directory. That means subsequent files in the same directory will not getCreateevents in this pass, even though they are perfectly valid.A more robust pattern is to skip only the problematic entry and continue scanning:
- fi, err := f.Info() - if err != nil { - if errors.Is(err, os.ErrNotExist) { - return nil - } - return fmt.Errorf("fsnotify.dirChange: %w", err) - } + fi, err := f.Info() + if err != nil { + if errors.Is(err, os.ErrNotExist) { + // This entry disappeared; skip it but keep processing others. + continue + } + return fmt.Errorf("fsnotify.dirChange: %w", err) + } @@ - err = w.sendCreateIfNew(filepath.Join(dir, fi.Name()), fi) - if err != nil { - // Don't need to send an error if this file isn't readable. - if errors.Is(err, unix.EACCES) || errors.Is(err, unix.EPERM) || errors.Is(err, os.ErrNotExist) { - return nil - } - return fmt.Errorf("fsnotify.dirChange: %w", err) - } + err = w.sendCreateIfNew(filepath.Join(dir, fi.Name()), fi) + if err != nil { + // Don't need to send an error if this file isn't readable. + if errors.Is(err, unix.EACCES) || errors.Is(err, unix.EPERM) || errors.Is(err, os.ErrNotExist) { + // Skip this entry but keep walking the directory. + continue + } + return fmt.Errorf("fsnotify.dirChange: %w", err) + }This preserves the intent of ignoring per‑file permission/disappearance issues without dropping events for other files in the same directory.
♻️ Duplicate comments (8)
vendor/github.com/google/cel-go/ext/README.md (1)
359-375: Math.Sqrt documentation contradicts behavior for negative inputs—duplicated concern.This issue was flagged in a previous review: the description states "Throws error for negative or non-numeric inputs" (line 364), but the example shows
math.sqrt(-15) // returns NaN(line 374). Go's math.Sqrt returns NaN when called with a negative number—it does not throw an error. Update the description to align with the example: "Returns NaN for negative numeric inputs; throws error for non-numeric inputs."Suggested fix:
-Throws error for negative or non-numeric inputs +Returns NaN for negative numeric inputs; throws error for non-numeric inputsvendor/github.com/google/cel-go/checker/decls/decls.go (1)
94-103: The unuseddocparameter issue persistsThis issue was already flagged in a previous review. The
NewFunctionWithDocfunction accepts adocparameter but never uses it (line 101 is commented out). Since this is vendored code fromgithub.com/google/cel-go, consider reporting this upstream if not already done, as it creates a misleading API surface.vendor/github.com/google/cel-go/ext/math.go (1)
328-344: Vendored code update approved; documentation inconsistency is an upstream issue.The documentation inconsistency flagged in previous reviews (line 334 claims errors for negative inputs, but line 344 example and implementation both show NaN is returned for negatives) remains present. However, this is vendored code from
github.com/google/cel-goand cannot be fixed directly in this PR. The actualsqrtimplementation is correct—it follows standard Gomath.Sqrtbehavior (returning NaN for negative numeric inputs per IEEE 754).If not already done, consider reporting the documentation issue to the upstream CEL-Go project.
Also applies to: 726-738
vendor/github.com/google/cel-go/ext/extension_option_factory.go (1)
24-42: Guard against nil*env.Extensionto avoid a potential panicIf
configElementis(*env.Extension)(nil), the type assertion succeeds (isExtension == true) butextis nil. Accessingext.Name,ext.Version, or callingext.VersionNumber()will then panic.A small defensive check keeps this robust against malformed configs:
func ExtensionOptionFactory(configElement any) (cel.EnvOption, bool) { - ext, isExtension := configElement.(*env.Extension) - if !isExtension { + ext, isExtension := configElement.(*env.Extension) + if !isExtension || ext == nil { return nil, false }This preserves current behavior for valid inputs while avoiding a crash if a nil extension is ever passed through.
vendor/github.com/google/cel-go/ext/strings.go (1)
346-354: StringsValidateFormatCalls is now a deprecated no‑op (behavior change)
StringsValidateFormatCallsignores its boolean and just returns the library unchanged, while validation is now always enabled forversion >= 1via the AST validators. Existing callers lose the ability to disable validation; this is effectively a behavioral change kept only for API compatibility.If you haven’t already, it’s worth making this explicit in higher‑level docs/release notes so consumers don’t expect the flag to do anything.
vendor/github.com/google/cel-go/cel/env.go (1)
180-215: Handle errors fromNewCustomEnv/Lib(lib)inToConfigto avoid panics
ToConfigcurrently ignores both errors and may leavelibEnvnil before callinglibEnv.Functions():libEnv, _ := NewCustomEnv() libEnv, _ = Lib(lib)(libEnv) for fnName, fnDecl := range libEnv.Functions() { ... }If either call fails, this will panic. Please propagate these errors (or at least bail out) before iterating:
- libEnv, _ := NewCustomEnv() - libEnv, _ = Lib(lib)(libEnv) + libEnv, err := NewCustomEnv() + if err != nil { + return nil, fmt.Errorf("failed to construct library env for %q: %w", libName, err) + } + libEnv, err = Lib(lib)(libEnv) + if err != nil { + return nil, fmt.Errorf("failed to apply library %q: %w", libName, err) + }This keeps
ToConfigfailure modes predictable and avoids nil-dereference crashes if a library setup ever fails.vendor/github.com/google/cel-go/common/env/env.go (2)
257-267: Nil pointer dereference risk remains unaddressed.This issue was previously flagged:
v.GetType()can returnnil(when bothv.TypeDescandv.Typeare nil), and callingValidate()on a nil pointer will panic instead of returning a validation error.Since this is vendored code from
google/cel-go, the fix should be applied upstream first.Please verify whether the upstream
google/cel-gorepository has addressed this issue in a newer version:#!/bin/bash # Check the upstream version being vendored and compare with latest echo "=== Checking vendored version ===" grep "github.com/google/cel-go" go.mod echo -e "\n=== Checking for related upstream issues/PRs ===" gh search issues --repo google/cel-go "GetType Validate nil" --limit 5 echo -e "\n=== Checking latest releases ===" gh release list --repo google/cel-go --limit 5
399-421: Nil pointer dereference risks remain despite "addressed" status.This issue was previously flagged and marked as "✅ Addressed in commits 5cf34cd to c7f7c0d", but the code still contains the problems:
- Line 413:
arg.Validate()is called without checking ifargis nil- Line 417:
od.Return.Validate()is called without checking ifod.Returnis nilThese can panic when YAML configuration omits arg or return types. Since this is vendored code from
google/cel-go, the fix should be applied upstream first.The previous "addressed" status appears inconsistent with the current code. Please verify:
#!/bin/bash # Check if the fix exists in commits mentioned echo "=== Checking commits 5cf34cd to c7f7c0d ===" git log --oneline --grep="Overload\|Validate\|nil" 5cf34cd..c7f7c0d 2>/dev/null || echo "Commits not found in current branch" echo -e "\n=== Checking current vendored code for nil checks ===" rg -n "for.*arg.*range.*Args" vendor/github.com/google/cel-go/common/env/env.go -A 3 -B 1 rg -n "od\.Return\.Validate" vendor/github.com/google/cel-go/common/env/env.go -A 1 -B 1 echo -e "\n=== Checking upstream for fixes ===" gh search issues --repo google/cel-go "Overload Validate nil args return" --limit 5
🧹 Nitpick comments (14)
vendor/github.com/google/cel-go/ext/README.md (1)
415-415: Replace bold emphasis with proper Markdown headings (MD036).Multiple sections use bold text instead of heading syntax, violating Markdown conventions. Each should be a level-4 heading (
####) or adjusted to match the documentation style. For example:
- Line 415:
**Introduced in version 2 (cost support in version 3)**- Line 429:
**Introduced in version 1 (cost support in version 3)**- Line 448, 461, 474, 487, 504: similar pattern
Apply this pattern to all occurrences:
-**Introduced in version X (cost support in version Y)** +#### Introduced in version X (cost support in version Y)Also applies to: 429-429, 448-448, 461-461, 474-474, 487-487, 504-504
vendor/github.com/google/cel-go/cel/io.go (1)
105-109: New ExprToString helper is a straightforward, useful exportThe exported
ExprToStringwrapper aroundparser.Unparselooks correct and makes expression unparsing reusable from anast.Expr+SourceInfo. Only tiny nit (optional): the doc comment is split with a blank//line in the middle of a sentence; if you touch this again, consider collapsing it into a single coherent paragraph.vendor/github.com/google/cel-go/common/stdlib/standard.go (1)
945-1058: Time-zone helpers look correct; optional robustness considerationsThe new
timestampGet*helpers andinTimeZonecorrectly:
- Reuse a shared
inTimeZonepath with either IANA locations or(+|-)HH:MM-style offsets.- Implement 0-based month/day-of-year/day-of-month and 1-based date semantics as per the docs.
- Derive fixed-offset zones from minute offsets without overflow risk in the documented range.
Two optional hardening ideas if you expect user-supplied tz strings to be noisy:
- Add defensive type checks in
inTimeZone(e.g., verifytsistypes.Timestampandtzistypes.Stringwith safe casts) instead of relying solely on the type system to avoid panics.- Enforce the documented offset format (e.g., simple validation before parsing), so obviously invalid strings fail with a clearer error rather than producing a surprising offset.
Current implementation is functionally sound as-is; these would just improve resilience to bad inputs.
vendor/github.com/fxamacker/cbor/v2/bytestring.go (1)
41-62: ByteString unmarshal flow with explicit well-formedness check looks soundSplitting
ByteStringdecoding into:
- an exported
UnmarshalCBORthat validates well‑formedness viadecoder.wellformed, and- an internal
unmarshalCBORthat assumes well‑formed data, handles null/undefined as empty, and enforces the byte‑string type,preserves existing semantics while making direct uses of
UnmarshalCBORconsistent with package‑levelUnmarshal. The duplicate nil‑receiver guard in both methods is safe (just slightly redundant) and not a blocker.Also applies to: 64-90
vendor/github.com/fxamacker/cbor/v2/simplevalue.go (1)
50-72: SimpleValue unmarshal changes correctly separate validation from decodingThe new pattern where
SimpleValue.UnmarshalCBOR:
- performs a nil‑receiver check,
- validates the CBOR item with
wellformed, then- defers to an internal
unmarshalCBORthat assumes well‑formed data and enforces the simple‑value type,is consistent with the ByteString changes and keeps exported behavior backwards compatible while tightening validation. The small duplication between the two methods (nil check and decoder creation) is acceptable here and can be refactored later if desired.
Also applies to: 74-83, 84-98
vendor/github.com/fxamacker/cbor/v2/encode.go (1)
439-447: Minor doc fixes (optional).
- “FieldNameToTextString encodes … byte string” comment is mismatched; should reference FieldNameToByteString.
- “IEEE 764” -> “IEEE 754”.
- // FieldNameToTextString encodes struct fields to CBOR byte string (major type 2). + // FieldNameToByteString encodes struct fields to CBOR byte string (major type 2).- // the 64-bit floating point value. I.e., the floating point encoding can be IEEE 764: + // the 64-bit floating point value. I.e., the floating point encoding can be IEEE 754:Also applies to: 331-355
pkg/conversion/capi2mapi/aws_fuzz_test.go (1)
31-36: HostPlacement fuzzing looks good; consider clearing HostID and reusing helper for templatesThe new HostPlacement fuzzing nicely exercises
HostAffinity/HostIDcombinations and uses a valid-looking host ID.Two optional improvements to consider:
- When
HostAffinityisnilor"default",HostIDmay still hold a random value fromc.FillNoCustom(spec). If the conversion logic expectsHostIDto be nil unless affinity is"host", you might want to explicitly resetspec.HostID = nilin those branches to better represent valid “happy path” states.- For parity, you could reuse
fuzzAWSMachineSpecHostPlacementinsideawsMachineTemplateFuzzerFuncs(e.g., against&m.Spec.Template.Spec) so MachineSet/Template paths get the same HostPlacement coverage as standalone AWSMachine fuzzing.Also applies to: 157-174, 211-221
vendor/github.com/google/cel-go/common/types/format.go (1)
1-42: New Format API is coherent; consider a small tweak to the fallback pathThe
Format/formatTodesign is solid: it lets built-ins implementformattable, then gracefully falls back to list/map traits, and finally to generic formatting.If you ever want to polish edge cases further, you might:
- Treat a nil
ref.Valspecially (e.g., emit"null"/"<nil>"), and/or- Use
%vinstead of%sin the finalfmt.Fprintfto avoid%!s(<nil>)-style diagnostics for non-string values.Purely optional; current behavior is acceptable for a best-effort human-readable formatter.
pkg/conversion/mapi2capi/aws_test.go (1)
96-121: HostPlacement AnyAvailable/DedicatedHost test entries look good; consider negative cases if not covered elsewhereFolding the dedicated host scenarios into the main DescribeTable and exercising both
HostAffinityAnyAvailableandHostAffinityDedicatedHostwith a realistic host ID gives nice coverage of the supported paths and keeps expectations (no errors/warnings) explicit.If negative HostPlacement cases (e.g., missing/invalid host ID, unsupported affinity) are not already covered in other tests, it could be worth adding a couple of entries here to lock in the failure behavior as well. Otherwise this section looks solid.
Also applies to: 429-430
vendor/github.com/google/cel-go/ext/strings.go (1)
349-350: Use Go’s standard deprecation comment formThe lone
// Deprecatedmarker won’t be picked up by Go tooling. To integrate withgo docand linters, it should start withDeprecated:and ideally explain the behavior.For example:
-// Deprecated -func StringsValidateFormatCalls(value bool) StringsOption { +// Deprecated: StringsValidateFormatCalls is a no-op; validation is always enabled +// for string.format() when version >= 1. Calls can be safely removed. +func StringsValidateFormatCalls(value bool) StringsOption {vendor/github.com/google/cel-go/cel/env.go (1)
589-592: Return a true copy fromValidators()to avoid external mutation
Validators()currently returns a slice header pointing at the original backing array:func (e *Env) Validators() []ASTValidator { return e.validators[:] }Callers can mutate this slice (append, re-order, replace elements) and affect the environment’s internal state.
For consistency with
Functions()andVariables(), consider copying into a new slice:func (e *Env) Validators() []ASTValidator { - return e.validators[:] + shallowCopy := make([]ASTValidator, len(e.validators)) + copy(shallowCopy, e.validators) + return shallowCopy }This keeps
Env’s validator configuration encapsulated.vendor/github.com/google/cel-go/interpreter/interpreter.go (1)
260-272: Defensively guard againstPlannerOptionreturning nil inNewInterpretable
NewInterpretableapplies eachPlannerOptionin sequence:p := newPlanner(...) var err error for _, o := range opts { p, err = o(p) if err != nil { return nil, err } } return p.Plan(checked.Expr())If a
PlannerOptionaccidentally returnsnilwithout error,p.Planwill panic. SincePlannerOptionis now part of the public surface, it’s cheap to harden this:for _, o := range opts { p, err = o(p) if err != nil { return nil, err } + if p == nil { + return nil, errors.New("planner option returned nil planner") + } }This keeps failures in user-supplied planner options as normal errors instead of panics.
vendor/github.com/google/cel-go/ext/formatting.go (1)
705-713:formatError.Iscompares by message; consider including the id for disambiguation
formatError.Iscurrently treats two errors as equal if their messages match:func (e formatError) Is(target error) bool { return e.msg == target.Error() }This ignores the
idfield, so distinct errors on different expressions that happen to share the same message will be indistinguishable toerrors.Is.If you rely on
errors.Isfor matching specific format errors, consider also checkingid(e.g., asserttargetis aformatErrorand compare both fields). If not, this is fine but worth being aware of.vendor/github.com/fsnotify/fsnotify/CHANGELOG.md (1)
3-4: Markdown heading style inconsistency (vendored file).Static analysis flagged that lines 3–4 and line 36 use setext-style headings (underlines with dashes) instead of ATX-style (
##). However, the entire file consistently uses setext style, so this is not an isolated issue. If the project enforces ATX-style headings, this would require a full file reformat rather than a local change. Since this is a vendored dependency file, verify whether it should be manually edited at all, or if it's typically regenerated/updated from upstream.Consider whether vendored changelogs are maintained manually or auto-generated from upstream. If manual edits are needed, ensure consistency with the file's existing style or standardize across the entire file.
Also applies to: 36-36
📜 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 ignored due to path filters (6)
e2e/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumhack/tools/go.sumis excluded by!**/*.summanifests-gen/go.sumis excluded by!**/*.sumvendor/cel.dev/expr/eval.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (107)
e2e/go.mod(5 hunks)go.mod(7 hunks)hack/tools/go.mod(4 hunks)manifests-gen/go.mod(5 hunks)manifests/0000_30_cluster-api_01_credentials-request.yaml(1 hunks)pkg/conversion/capi2mapi/aws.go(3 hunks)pkg/conversion/capi2mapi/aws_fuzz_test.go(3 hunks)pkg/conversion/capi2mapi/aws_test.go(6 hunks)pkg/conversion/capi2mapi/machine_test.go(1 hunks)pkg/conversion/capi2mapi/openstack_test.go(6 hunks)pkg/conversion/capi2mapi/powervs_test.go(3 hunks)pkg/conversion/mapi2capi/aws.go(2 hunks)pkg/conversion/mapi2capi/aws_fuzz_test.go(3 hunks)pkg/conversion/mapi2capi/aws_test.go(8 hunks)pkg/conversion/mapi2capi/machine_test.go(1 hunks)pkg/conversion/mapi2capi/powervs_test.go(2 hunks)vendor/cel.dev/expr/MODULE.bazel(4 hunks)vendor/github.com/fsnotify/fsnotify/.cirrus.yml(1 hunks)vendor/github.com/fsnotify/fsnotify/CHANGELOG.md(1 hunks)vendor/github.com/fsnotify/fsnotify/CONTRIBUTING.md(1 hunks)vendor/github.com/fsnotify/fsnotify/README.md(0 hunks)vendor/github.com/fsnotify/fsnotify/backend_fen.go(13 hunks)vendor/github.com/fsnotify/fsnotify/backend_inotify.go(13 hunks)vendor/github.com/fsnotify/fsnotify/backend_kqueue.go(13 hunks)vendor/github.com/fsnotify/fsnotify/backend_other.go(1 hunks)vendor/github.com/fsnotify/fsnotify/backend_windows.go(6 hunks)vendor/github.com/fsnotify/fsnotify/fsnotify.go(3 hunks)vendor/github.com/fsnotify/fsnotify/internal/darwin.go(1 hunks)vendor/github.com/fsnotify/fsnotify/internal/freebsd.go(1 hunks)vendor/github.com/fsnotify/fsnotify/internal/unix.go(2 hunks)vendor/github.com/fsnotify/fsnotify/internal/windows.go(1 hunks)vendor/github.com/fsnotify/fsnotify/shared.go(1 hunks)vendor/github.com/fsnotify/fsnotify/staticcheck.conf(1 hunks)vendor/github.com/fxamacker/cbor/v2/README.md(14 hunks)vendor/github.com/fxamacker/cbor/v2/bytestring.go(1 hunks)vendor/github.com/fxamacker/cbor/v2/cache.go(7 hunks)vendor/github.com/fxamacker/cbor/v2/common.go(2 hunks)vendor/github.com/fxamacker/cbor/v2/decode.go(42 hunks)vendor/github.com/fxamacker/cbor/v2/doc.go(5 hunks)vendor/github.com/fxamacker/cbor/v2/encode.go(22 hunks)vendor/github.com/fxamacker/cbor/v2/encode_map.go(1 hunks)vendor/github.com/fxamacker/cbor/v2/encode_map_go117.go(0 hunks)vendor/github.com/fxamacker/cbor/v2/omitzero_go124.go(1 hunks)vendor/github.com/fxamacker/cbor/v2/omitzero_pre_go124.go(1 hunks)vendor/github.com/fxamacker/cbor/v2/simplevalue.go(2 hunks)vendor/github.com/fxamacker/cbor/v2/stream.go(2 hunks)vendor/github.com/fxamacker/cbor/v2/structfields.go(7 hunks)vendor/github.com/fxamacker/cbor/v2/tag.go(4 hunks)vendor/github.com/google/cel-go/cel/BUILD.bazel(5 hunks)vendor/github.com/google/cel-go/cel/decls.go(4 hunks)vendor/github.com/google/cel-go/cel/env.go(13 hunks)vendor/github.com/google/cel-go/cel/folding.go(9 hunks)vendor/github.com/google/cel-go/cel/io.go(2 hunks)vendor/github.com/google/cel-go/cel/library.go(15 hunks)vendor/github.com/google/cel-go/cel/macro.go(1 hunks)vendor/github.com/google/cel-go/cel/options.go(11 hunks)vendor/github.com/google/cel-go/cel/program.go(14 hunks)vendor/github.com/google/cel-go/cel/prompt.go(1 hunks)vendor/github.com/google/cel-go/cel/templates/authoring.tmpl(1 hunks)vendor/github.com/google/cel-go/cel/validator.go(6 hunks)vendor/github.com/google/cel-go/checker/checker.go(1 hunks)vendor/github.com/google/cel-go/checker/cost.go(1 hunks)vendor/github.com/google/cel-go/checker/decls/decls.go(3 hunks)vendor/github.com/google/cel-go/checker/errors.go(1 hunks)vendor/github.com/google/cel-go/common/BUILD.bazel(2 hunks)vendor/github.com/google/cel-go/common/ast/ast.go(2 hunks)vendor/github.com/google/cel-go/common/ast/navigable.go(1 hunks)vendor/github.com/google/cel-go/common/containers/container.go(4 hunks)vendor/github.com/google/cel-go/common/decls/BUILD.bazel(1 hunks)vendor/github.com/google/cel-go/common/decls/decls.go(24 hunks)vendor/github.com/google/cel-go/common/doc.go(1 hunks)vendor/github.com/google/cel-go/common/env/BUILD.bazel(1 hunks)vendor/github.com/google/cel-go/common/env/env.go(1 hunks)vendor/github.com/google/cel-go/common/stdlib/BUILD.bazel(1 hunks)vendor/github.com/google/cel-go/common/stdlib/standard.go(10 hunks)vendor/github.com/google/cel-go/common/types/BUILD.bazel(1 hunks)vendor/github.com/google/cel-go/common/types/bool.go(2 hunks)vendor/github.com/google/cel-go/common/types/bytes.go(2 hunks)vendor/github.com/google/cel-go/common/types/double.go(2 hunks)vendor/github.com/google/cel-go/common/types/duration.go(2 hunks)vendor/github.com/google/cel-go/common/types/format.go(1 hunks)vendor/github.com/google/cel-go/common/types/int.go(2 hunks)vendor/github.com/google/cel-go/common/types/list.go(1 hunks)vendor/github.com/google/cel-go/common/types/map.go(2 hunks)vendor/github.com/google/cel-go/common/types/null.go(2 hunks)vendor/github.com/google/cel-go/common/types/object.go(2 hunks)vendor/github.com/google/cel-go/common/types/optional.go(2 hunks)vendor/github.com/google/cel-go/common/types/string.go(1 hunks)vendor/github.com/google/cel-go/common/types/timestamp.go(1 hunks)vendor/github.com/google/cel-go/common/types/types.go(3 hunks)vendor/github.com/google/cel-go/common/types/uint.go(2 hunks)vendor/github.com/google/cel-go/ext/BUILD.bazel(4 hunks)vendor/github.com/google/cel-go/ext/README.md(8 hunks)vendor/github.com/google/cel-go/ext/bindings.go(4 hunks)vendor/github.com/google/cel-go/ext/extension_option_factory.go(1 hunks)vendor/github.com/google/cel-go/ext/formatting.go(18 hunks)vendor/github.com/google/cel-go/ext/formatting_v2.go(1 hunks)vendor/github.com/google/cel-go/ext/lists.go(13 hunks)vendor/github.com/google/cel-go/ext/math.go(4 hunks)vendor/github.com/google/cel-go/ext/native.go(1 hunks)vendor/github.com/google/cel-go/ext/regex.go(1 hunks)vendor/github.com/google/cel-go/ext/sets.go(2 hunks)vendor/github.com/google/cel-go/ext/strings.go(7 hunks)vendor/github.com/google/cel-go/interpreter/activation.go(2 hunks)vendor/github.com/google/cel-go/interpreter/attribute_patterns.go(1 hunks)vendor/github.com/google/cel-go/interpreter/interpretable.go(13 hunks)vendor/github.com/google/cel-go/interpreter/interpreter.go(5 hunks)
⛔ Files not processed due to max files limit (40)
- vendor/github.com/google/cel-go/interpreter/planner.go
- vendor/github.com/google/cel-go/interpreter/runtimecost.go
- vendor/github.com/google/cel-go/parser/macro.go
- vendor/github.com/google/gnostic-models/compiler/context.go
- vendor/github.com/google/gnostic-models/compiler/extensions.go
- vendor/github.com/google/gnostic-models/compiler/helpers.go
- vendor/github.com/google/gnostic-models/compiler/reader.go
- vendor/github.com/google/gnostic-models/jsonschema/models.go
- vendor/github.com/google/gnostic-models/jsonschema/reader.go
- vendor/github.com/google/gnostic-models/jsonschema/writer.go
- vendor/github.com/google/gnostic-models/openapiv2/OpenAPIv2.go
- vendor/github.com/google/gnostic-models/openapiv2/document.go
- vendor/github.com/google/gnostic-models/openapiv3/OpenAPIv3.go
- vendor/github.com/google/gnostic-models/openapiv3/document.go
- vendor/github.com/modern-go/reflect2/safe_type.go
- vendor/github.com/openshift/api/.ci-operator.yaml
- vendor/github.com/openshift/api/.golangci.go-validated.yaml
- vendor/github.com/openshift/api/.golangci.yaml
- vendor/github.com/openshift/api/AGENTS.md
- vendor/github.com/openshift/api/Dockerfile.ocp
- vendor/github.com/openshift/api/Makefile
- vendor/github.com/openshift/api/OWNERS
- vendor/github.com/openshift/api/config/v1/register.go
- vendor/github.com/openshift/api/config/v1/types_apiserver.go
- vendor/github.com/openshift/api/config/v1/types_authentication.go
- vendor/github.com/openshift/api/config/v1/types_cluster_operator.go
- vendor/github.com/openshift/api/config/v1/types_cluster_version.go
- vendor/github.com/openshift/api/config/v1/types_infrastructure.go
- vendor/github.com/openshift/api/config/v1/types_insights.go
- vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusteroperators.crd.yaml
- vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-CustomNoUpgrade.crd.yaml
- vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-Default.crd.yaml
- vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-DevPreviewNoUpgrade.crd.yaml
- vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-TechPreviewNoUpgrade.crd.yaml
- vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml
- vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yaml
- vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
- vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
- vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-Hypershift-CustomNoUpgrade.crd.yaml
- vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-Hypershift-Default.crd.yaml
💤 Files with no reviewable changes (2)
- vendor/github.com/fsnotify/fsnotify/README.md
- vendor/github.com/fxamacker/cbor/v2/encode_map_go117.go
✅ Files skipped from review due to trivial changes (1)
- pkg/conversion/mapi2capi/machine_test.go
🚧 Files skipped from review as they are similar to previous changes (28)
- vendor/github.com/fsnotify/fsnotify/.cirrus.yml
- vendor/github.com/fsnotify/fsnotify/internal/windows.go
- vendor/github.com/fsnotify/fsnotify/staticcheck.conf
- pkg/conversion/capi2mapi/aws_test.go
- pkg/conversion/capi2mapi/powervs_test.go
- vendor/github.com/fxamacker/cbor/v2/omitzero_pre_go124.go
- vendor/github.com/google/cel-go/common/ast/ast.go
- vendor/github.com/google/cel-go/cel/templates/authoring.tmpl
- vendor/github.com/google/cel-go/common/types/null.go
- vendor/cel.dev/expr/MODULE.bazel
- hack/tools/go.mod
- pkg/conversion/capi2mapi/openstack_test.go
- vendor/github.com/fsnotify/fsnotify/CONTRIBUTING.md
- vendor/github.com/google/cel-go/common/decls/BUILD.bazel
- vendor/github.com/google/cel-go/ext/formatting_v2.go
- vendor/github.com/fsnotify/fsnotify/fsnotify.go
- vendor/github.com/google/cel-go/common/BUILD.bazel
- vendor/github.com/google/cel-go/common/types/BUILD.bazel
- vendor/github.com/google/cel-go/ext/sets.go
- manifests/0000_30_cluster-api_01_credentials-request.yaml
- vendor/github.com/fxamacker/cbor/v2/omitzero_go124.go
- vendor/github.com/google/cel-go/checker/cost.go
- vendor/github.com/google/cel-go/common/types/object.go
- pkg/conversion/capi2mapi/aws.go
- vendor/github.com/fsnotify/fsnotify/internal/darwin.go
- vendor/github.com/google/cel-go/ext/bindings.go
- vendor/github.com/fsnotify/fsnotify/backend_other.go
- vendor/github.com/google/cel-go/common/types/bool.go
🧰 Additional context used
🪛 LanguageTool
vendor/github.com/fxamacker/cbor/v2/README.md
[style] ~7-~7: In American English, abbreviations like “etc.” require a period.
Context: ...Averaison+fxamacker%2Fcbor&type=code), [etc](https://github.com/fxamacker/cbor#who-...
(ETC_PERIOD)
[style] ~726-~726: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ...5.0) was released on Sunday, August 13, 2023 with new features and important bug fix...
(MISSING_COMMA_AFTER_YEAR)
🪛 markdownlint-cli2 (0.18.1)
vendor/github.com/google/cel-go/ext/README.md
415-415: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
429-429: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
448-448: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
461-461: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
474-474: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
487-487: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
504-504: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
vendor/github.com/fxamacker/cbor/v2/README.md
13-13: Images should have alternate text (alt text)
(MD045, no-alt-text)
14-14: Images should have alternate text (alt text)
(MD045, no-alt-text)
16-16: Images should have alternate text (alt text)
(MD045, no-alt-text)
18-18: Images should have alternate text (alt text)
(MD045, no-alt-text)
108-108: Hard tabs
Column: 3
(MD010, no-hard-tabs)
109-109: Hard tabs
Column: 3
(MD010, no-hard-tabs)
110-110: Hard tabs
Column: 3
(MD010, no-hard-tabs)
111-111: Hard tabs
Column: 3
(MD010, no-hard-tabs)
117-117: Hard tabs
Column: 3
(MD010, no-hard-tabs)
118-118: Hard tabs
Column: 3
(MD010, no-hard-tabs)
119-119: Hard tabs
Column: 3
(MD010, no-hard-tabs)
120-120: Hard tabs
Column: 3
(MD010, no-hard-tabs)
121-121: Hard tabs
Column: 3
(MD010, no-hard-tabs)
122-122: Hard tabs
Column: 3
(MD010, no-hard-tabs)
125-125: Hard tabs
Column: 3
(MD010, no-hard-tabs)
126-126: Hard tabs
Column: 3
(MD010, no-hard-tabs)
130-130: Hard tabs
Column: 3
(MD010, no-hard-tabs)
131-131: Hard tabs
Column: 3
(MD010, no-hard-tabs)
133-133: Hard tabs
Column: 3
(MD010, no-hard-tabs)
134-134: Hard tabs
Column: 3
(MD010, no-hard-tabs)
135-135: Hard tabs
Column: 3
(MD010, no-hard-tabs)
159-159: Multiple spaces after blockquote symbol
(MD027, no-multiple-space-blockquote)
165-165: Bare URL used
(MD034, no-bare-urls)
175-175: Hard tabs
Column: 3
(MD010, no-hard-tabs)
176-176: Hard tabs
Column: 3
(MD010, no-hard-tabs)
177-177: Hard tabs
Column: 3
(MD010, no-hard-tabs)
179-179: Hard tabs
Column: 3
(MD010, no-hard-tabs)
183-183: Hard tabs
Column: 3
(MD010, no-hard-tabs)
187-187: Hard tabs
Column: 3
(MD010, no-hard-tabs)
188-188: Hard tabs
Column: 3
(MD010, no-hard-tabs)
192-192: Hard tabs
Column: 3
(MD010, no-hard-tabs)
193-193: Hard tabs
Column: 3
(MD010, no-hard-tabs)
197-197: Hard tabs
Column: 3
(MD010, no-hard-tabs)
198-198: Hard tabs
Column: 3
(MD010, no-hard-tabs)
200-200: Hard tabs
Column: 3
(MD010, no-hard-tabs)
201-201: Hard tabs
Column: 3
(MD010, no-hard-tabs)
205-205: Hard tabs
Column: 3
(MD010, no-hard-tabs)
206-206: Hard tabs
Column: 3
(MD010, no-hard-tabs)
208-208: Hard tabs
Column: 3
(MD010, no-hard-tabs)
209-209: Hard tabs
Column: 3
(MD010, no-hard-tabs)
213-213: Hard tabs
Column: 3
(MD010, no-hard-tabs)
214-214: Hard tabs
Column: 3
(MD010, no-hard-tabs)
215-215: Hard tabs
Column: 3
(MD010, no-hard-tabs)
220-220: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
255-255: Bare URL used
(MD034, no-bare-urls)
256-256: Bare URL used
(MD034, no-bare-urls)
364-364: Bare URL used
(MD034, no-bare-urls)
371-371: Hard tabs
Column: 1
(MD010, no-hard-tabs)
372-372: Hard tabs
Column: 1
(MD010, no-hard-tabs)
374-374: Hard tabs
Column: 1
(MD010, no-hard-tabs)
379-379: Hard tabs
Column: 1
(MD010, no-hard-tabs)
380-380: Hard tabs
Column: 1
(MD010, no-hard-tabs)
381-381: Hard tabs
Column: 1
(MD010, no-hard-tabs)
382-382: Hard tabs
Column: 1
(MD010, no-hard-tabs)
386-386: Hard tabs
Column: 1
(MD010, no-hard-tabs)
387-387: Hard tabs
Column: 1
(MD010, no-hard-tabs)
388-388: Hard tabs
Column: 1
(MD010, no-hard-tabs)
389-389: Hard tabs
Column: 1
(MD010, no-hard-tabs)
390-390: Hard tabs
Column: 1
(MD010, no-hard-tabs)
392-392: Hard tabs
Column: 1
(MD010, no-hard-tabs)
393-393: Hard tabs
Column: 1
(MD010, no-hard-tabs)
394-394: Hard tabs
Column: 1
(MD010, no-hard-tabs)
395-395: Hard tabs
Column: 1
(MD010, no-hard-tabs)
397-397: Hard tabs
Column: 1
(MD010, no-hard-tabs)
398-398: Hard tabs
Column: 1
(MD010, no-hard-tabs)
400-400: Hard tabs
Column: 1
(MD010, no-hard-tabs)
401-401: Hard tabs
Column: 1
(MD010, no-hard-tabs)
403-403: Hard tabs
Column: 1
(MD010, no-hard-tabs)
404-404: Hard tabs
Column: 1
(MD010, no-hard-tabs)
405-405: Hard tabs
Column: 1
(MD010, no-hard-tabs)
406-406: Hard tabs
Column: 1
(MD010, no-hard-tabs)
407-407: Hard tabs
Column: 1
(MD010, no-hard-tabs)
408-408: Hard tabs
Column: 1
(MD010, no-hard-tabs)
483-483: Hard tabs
Column: 1
(MD010, no-hard-tabs)
531-531: Hard tabs
Column: 1
(MD010, no-hard-tabs)
554-554: Hard tabs
Column: 3
(MD010, no-hard-tabs)
555-555: Hard tabs
Column: 3
(MD010, no-hard-tabs)
556-556: Hard tabs
Column: 3
(MD010, no-hard-tabs)
562-562: Hard tabs
Column: 1
(MD010, no-hard-tabs)
563-563: Hard tabs
Column: 1
(MD010, no-hard-tabs)
564-564: Hard tabs
Column: 1
(MD010, no-hard-tabs)
566-566: Hard tabs
Column: 1
(MD010, no-hard-tabs)
576-576: Hard tabs
Column: 1
(MD010, no-hard-tabs)
580-580: Hard tabs
Column: 1
(MD010, no-hard-tabs)
587-587: Hard tabs
Column: 1
(MD010, no-hard-tabs)
588-588: Hard tabs
Column: 1
(MD010, no-hard-tabs)
589-589: Hard tabs
Column: 1
(MD010, no-hard-tabs)
590-590: Hard tabs
Column: 1
(MD010, no-hard-tabs)
591-591: Hard tabs
Column: 1
(MD010, no-hard-tabs)
593-593: Hard tabs
Column: 1
(MD010, no-hard-tabs)
594-594: Hard tabs
Column: 1
(MD010, no-hard-tabs)
595-595: Hard tabs
Column: 1
(MD010, no-hard-tabs)
596-596: Hard tabs
Column: 1
(MD010, no-hard-tabs)
597-597: Hard tabs
Column: 1
(MD010, no-hard-tabs)
599-599: Hard tabs
Column: 1
(MD010, no-hard-tabs)
600-600: Hard tabs
Column: 1
(MD010, no-hard-tabs)
608-608: Hard tabs
Column: 1
(MD010, no-hard-tabs)
609-609: Hard tabs
Column: 1
(MD010, no-hard-tabs)
610-610: Hard tabs
Column: 1
(MD010, no-hard-tabs)
611-611: Hard tabs
Column: 1
(MD010, no-hard-tabs)
612-612: Hard tabs
Column: 1
(MD010, no-hard-tabs)
614-614: Hard tabs
Column: 1
(MD010, no-hard-tabs)
615-615: Hard tabs
Column: 1
(MD010, no-hard-tabs)
616-616: Hard tabs
Column: 1
(MD010, no-hard-tabs)
617-617: Hard tabs
Column: 1
(MD010, no-hard-tabs)
619-619: Hard tabs
Column: 1
(MD010, no-hard-tabs)
620-620: Hard tabs
Column: 1
(MD010, no-hard-tabs)
621-621: Hard tabs
Column: 1
(MD010, no-hard-tabs)
622-622: Hard tabs
Column: 1
(MD010, no-hard-tabs)
623-623: Hard tabs
Column: 1
(MD010, no-hard-tabs)
625-625: Hard tabs
Column: 1
(MD010, no-hard-tabs)
626-626: Hard tabs
Column: 1
(MD010, no-hard-tabs)
631-631: Hard tabs
Column: 1
(MD010, no-hard-tabs)
636-636: Hard tabs
Column: 1
(MD010, no-hard-tabs)
637-637: Hard tabs
Column: 1
(MD010, no-hard-tabs)
638-638: Hard tabs
Column: 1
(MD010, no-hard-tabs)
642-642: Hard tabs
Column: 1
(MD010, no-hard-tabs)
643-643: Hard tabs
Column: 1
(MD010, no-hard-tabs)
644-644: Hard tabs
Column: 1
(MD010, no-hard-tabs)
645-645: Hard tabs
Column: 1
(MD010, no-hard-tabs)
647-647: Hard tabs
Column: 1
(MD010, no-hard-tabs)
648-648: Hard tabs
Column: 1
(MD010, no-hard-tabs)
649-649: Hard tabs
Column: 1
(MD010, no-hard-tabs)
650-650: Hard tabs
Column: 1
(MD010, no-hard-tabs)
652-652: Hard tabs
Column: 1
(MD010, no-hard-tabs)
654-654: Hard tabs
Column: 1
(MD010, no-hard-tabs)
655-655: Hard tabs
Column: 1
(MD010, no-hard-tabs)
656-656: Hard tabs
Column: 1
(MD010, no-hard-tabs)
657-657: Hard tabs
Column: 1
(MD010, no-hard-tabs)
658-658: Hard tabs
Column: 1
(MD010, no-hard-tabs)
660-660: Hard tabs
Column: 1
(MD010, no-hard-tabs)
661-661: Hard tabs
Column: 1
(MD010, no-hard-tabs)
662-662: Hard tabs
Column: 1
(MD010, no-hard-tabs)
663-663: Hard tabs
Column: 1
(MD010, no-hard-tabs)
vendor/github.com/fsnotify/fsnotify/CHANGELOG.md
3-3: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
36-36: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
| func (w *kqueue) Close() error { | ||
| w.doneMu.Lock() | ||
| if w.isClosed() { | ||
| w.doneMu.Unlock() | ||
| if w.shared.close() { | ||
| return nil | ||
| } | ||
| close(w.done) | ||
| w.doneMu.Unlock() | ||
|
|
||
| pathsToRemove := w.watches.listPaths(false) | ||
| for _, name := range pathsToRemove { | ||
| w.Remove(name) | ||
| } | ||
|
|
||
| // Send "quit" message to the reader goroutine. | ||
| unix.Close(w.closepipe[1]) | ||
| unix.Close(w.closepipe[1]) // Send "quit" message to readEvents | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Closing kqueue marks watcher closed before removing watches, leaking fds
Close() currently does:
func (w *kqueue) Close() error {
if w.shared.close() {
return nil
}
pathsToRemove := w.watches.listPaths(false)
for _, name := range pathsToRemove {
w.Remove(name)
}
unix.Close(w.closepipe[1]) // Send "quit" message to readEvents
return nil
}But remove() immediately returns when the watcher is closed:
func (w *kqueue) remove(name string, unwatchFiles bool) error {
if w.isClosed() {
return nil
}
// ...
unix.Close(info.wd)
// ...
}Because shared.close() closes done, subsequent calls to w.Remove/w.remove see w.isClosed() == true and no‑op. That means the per‑path watch descriptors (info.wd) are never unregistered or closed, leaking file descriptors until the process exits.
Reorder Close() so that watches are removed before the shared closed state is set:
func (w *kqueue) Close() error {
- if w.shared.close() {
- return nil
- }
-
- pathsToRemove := w.watches.listPaths(false)
- for _, name := range pathsToRemove {
- w.Remove(name)
- }
-
- unix.Close(w.closepipe[1]) // Send "quit" message to readEvents
- return nil
+ if w.isClosed() {
+ return nil
+ }
+
+ pathsToRemove := w.watches.listPaths(false)
+ for _, name := range pathsToRemove {
+ _ = w.Remove(name) // Ignore ErrNonExistentWatch, consistent with current behavior.
+ }
+
+ if w.shared.close() {
+ // Already closed concurrently; nothing left to signal.
+ return nil
+ }
+
+ // Send "quit" message to readEvents.
+ unix.Close(w.closepipe[1])
+ return nil
}This ensures all watches are deregistered and their descriptors closed before the watcher is marked closed and the IO loop is signaled to exit, while keeping Close() idempotent.
Also applies to: 286-317
| func (p *Prompt) Render(userPrompt string) string { | ||
| var buffer strings.Builder | ||
| vars := make([]*common.Doc, len(p.env.Variables())) | ||
| for i, v := range p.env.Variables() { | ||
| vars[i] = v.Documentation() | ||
| } | ||
| sort.SliceStable(vars, func(i, j int) bool { | ||
| return vars[i].Name < vars[j].Name | ||
| }) | ||
| macs := make([]*common.Doc, len(p.env.Macros())) | ||
| for i, m := range p.env.Macros() { | ||
| macs[i] = m.(common.Documentor).Documentation() | ||
| } | ||
| funcs := make([]*common.Doc, 0, len(p.env.Functions())) | ||
| for _, f := range p.env.Functions() { | ||
| if _, hidden := hiddenFunctions[f.Name()]; hidden { | ||
| continue | ||
| } | ||
| funcs = append(funcs, f.Documentation()) | ||
| } | ||
| sort.SliceStable(funcs, func(i, j int) bool { | ||
| return funcs[i].Name < funcs[j].Name | ||
| }) | ||
| inst := &promptInst{ | ||
| Prompt: p, | ||
| Variables: vars, | ||
| Macros: macs, | ||
| Functions: funcs, | ||
| UserPrompt: userPrompt} | ||
| p.tmpl.Execute(&buffer, inst) | ||
| return buffer.String() | ||
| } |
There was a problem hiding this comment.
Missing error handling in template rendering.
Line 111 calls p.tmpl.Execute(&buffer, inst) but ignores the returned error. If template execution fails, the method returns a potentially incomplete or empty string with no indication of failure.
While this is vendored code from upstream, it's worth noting that callers have no way to detect rendering failures.
🤖 Prompt for AI Agents
In vendor/github.com/google/cel-go/cel/prompt.go around lines 82 to 113, the
call to p.tmpl.Execute(&buffer, inst) ignores its returned error; update Render
to return (string, error) instead of just string, capture the error from
p.tmpl.Execute, and if non-nil return an empty string and the error; update all
callers to handle the new (string, error) signature and propagate or log the
error as appropriate.
| if f == nil { | ||
| return fmt.Errorf("nil function cannot add overload: %s", overload.ID()) | ||
| } | ||
| if overload == nil { | ||
| return fmt.Errorf("cannot add nil overload to funciton: %s", f.Name()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Fix typo in AddOverload error message
if overload == nil {
return fmt.Errorf("cannot add nil overload to funciton: %s", f.Name())
}Minor nit: "funciton" → "function".
- return fmt.Errorf("cannot add nil overload to funciton: %s", f.Name())
+ return fmt.Errorf("cannot add nil overload to function: %s", f.Name())Purely cosmetic, but easy to fix.
🏁 Script executed:
#!/bin/bash
# Verify the typo in the vendor file
if [ -f "vendor/github.com/google/cel-go/common/decls/decls.go" ]; then
echo "File found. Checking lines 260-265:"
sed -n '260,265p' vendor/github.com/google/cel-go/common/decls/decls.go
echo ""
echo "Searching for 'funciton' typo:"
rg "funciton" vendor/github.com/google/cel-go/common/decls/decls.go -n
else
echo "File not found at expected path. Searching for decls.go files:"
find . -name "decls.go" -path "*/cel-go/*" 2>/dev/null | head -20
fiLength of output: 335
Fix typo in AddOverload error message
Line 264 of vendor/github.com/google/cel-go/common/decls/decls.go contains a typo in the error message: "funciton" should be "function".
- return fmt.Errorf("cannot add nil overload to funciton: %s", f.Name())
+ return fmt.Errorf("cannot add nil overload to function: %s", f.Name())📝 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.
| if f == nil { | |
| return fmt.Errorf("nil function cannot add overload: %s", overload.ID()) | |
| } | |
| if overload == nil { | |
| return fmt.Errorf("cannot add nil overload to funciton: %s", f.Name()) | |
| } | |
| if f == nil { | |
| return fmt.Errorf("nil function cannot add overload: %s", overload.ID()) | |
| } | |
| if overload == nil { | |
| return fmt.Errorf("cannot add nil overload to function: %s", f.Name()) | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/google/cel-go/common/decls/decls.go around lines 260 to
265, fix the typo in the error message by changing "funciton" to "function" so
the fmt.Errorf call reads "cannot add nil overload to function: %s" (use
f.Name() as before); update only the string literal to correct the spelling.
| // Timestamp / duration functions | ||
| function(overloads.TimeGetFullYear, | ||
| decls.FunctionDocs(`get the 0-based full year from a timestamp, UTC unless an IANA timezone is specified.`), | ||
| decls.MemberOverload(overloads.TimestampToYear, | ||
| argTypes(types.TimestampType), types.IntType), | ||
| argTypes(types.TimestampType), types.IntType, | ||
| decls.OverloadExamples(`timestamp('2023-07-14T10:30:45.123Z').getFullYear() // 2023`), | ||
| decls.UnaryBinding(func(ts ref.Val) ref.Val { | ||
| return timestampGetFullYear(ts, utcTZ) | ||
| })), | ||
| decls.MemberOverload(overloads.TimestampToYearWithTz, | ||
| argTypes(types.TimestampType, types.StringType), types.IntType)), | ||
| argTypes(types.TimestampType, types.StringType), types.IntType, | ||
| decls.OverloadExamples(`timestamp('2023-01-01T05:30:00Z').getFullYear('-08:00') // 2022`), | ||
| decls.BinaryBinding(timestampGetFullYear))), | ||
|
|
||
| function(overloads.TimeGetMonth, | ||
| decls.FunctionDocs(`get the 0-based month from a timestamp, UTC unless an IANA timezone is specified.`), | ||
| decls.MemberOverload(overloads.TimestampToMonth, | ||
| argTypes(types.TimestampType), types.IntType), | ||
| argTypes(types.TimestampType), types.IntType, | ||
| decls.OverloadExamples(`timestamp('2023-07-14T10:30:45.123Z').getMonth() // 6`), // July is month 6 | ||
| decls.UnaryBinding(func(ts ref.Val) ref.Val { | ||
| return timestampGetMonth(ts, utcTZ) | ||
| })), | ||
| decls.MemberOverload(overloads.TimestampToMonthWithTz, | ||
| argTypes(types.TimestampType, types.StringType), types.IntType)), | ||
| argTypes(types.TimestampType, types.StringType), types.IntType, | ||
| decls.OverloadExamples(`timestamp('2023-01-01T05:30:00Z').getMonth('America/Los_Angeles') // 11`), // December is month 11 | ||
| decls.BinaryBinding(timestampGetMonth))), | ||
|
|
||
| function(overloads.TimeGetDayOfYear, | ||
| decls.FunctionDocs(`get the 0-based day of the year from a timestamp, UTC unless an IANA timezone is specified.`), | ||
| decls.MemberOverload(overloads.TimestampToDayOfYear, | ||
| argTypes(types.TimestampType), types.IntType), | ||
| argTypes(types.TimestampType), types.IntType, | ||
| decls.OverloadExamples(`timestamp('2023-01-02T00:00:00Z').getDayOfYear() // 1`), | ||
| decls.UnaryBinding(func(ts ref.Val) ref.Val { | ||
| return timestampGetDayOfYear(ts, utcTZ) | ||
| })), | ||
| decls.MemberOverload(overloads.TimestampToDayOfYearWithTz, | ||
| argTypes(types.TimestampType, types.StringType), types.IntType)), | ||
| argTypes(types.TimestampType, types.StringType), types.IntType, | ||
| decls.OverloadExamples(`timestamp('2023-01-01T05:00:00Z').getDayOfYear('America/Los_Angeles') // 364`), | ||
| decls.BinaryBinding(timestampGetDayOfYear))), | ||
|
|
||
| function(overloads.TimeGetDayOfMonth, | ||
| decls.FunctionDocs(`get the 0-based day of the month from a timestamp, UTC unless an IANA timezone is specified.`), | ||
| decls.MemberOverload(overloads.TimestampToDayOfMonthZeroBased, | ||
| argTypes(types.TimestampType), types.IntType), | ||
| argTypes(types.TimestampType), types.IntType, | ||
| decls.OverloadExamples(`timestamp('2023-07-14T10:30:45.123Z').getDayOfMonth() // 13`), | ||
| decls.UnaryBinding(func(ts ref.Val) ref.Val { | ||
| return timestampGetDayOfMonthZeroBased(ts, utcTZ) | ||
| })), | ||
| decls.MemberOverload(overloads.TimestampToDayOfMonthZeroBasedWithTz, | ||
| argTypes(types.TimestampType, types.StringType), types.IntType)), | ||
| argTypes(types.TimestampType, types.StringType), types.IntType, | ||
| decls.OverloadExamples(`timestamp('2023-07-01T05:00:00Z').getDayOfMonth('America/Los_Angeles') // 29`), | ||
| decls.BinaryBinding(timestampGetDayOfMonthZeroBased))), | ||
|
|
||
| function(overloads.TimeGetDate, | ||
| decls.FunctionDocs(`get the 1-based day of the month from a timestamp, UTC unless an IANA timezone is specified.`), | ||
| decls.MemberOverload(overloads.TimestampToDayOfMonthOneBased, | ||
| argTypes(types.TimestampType), types.IntType), | ||
| argTypes(types.TimestampType), types.IntType, | ||
| decls.OverloadExamples(`timestamp('2023-07-14T10:30:45.123Z').getDate() // 14`), | ||
| decls.UnaryBinding(func(ts ref.Val) ref.Val { | ||
| return timestampGetDayOfMonthOneBased(ts, utcTZ) | ||
| })), | ||
| decls.MemberOverload(overloads.TimestampToDayOfMonthOneBasedWithTz, | ||
| argTypes(types.TimestampType, types.StringType), types.IntType)), | ||
| argTypes(types.TimestampType, types.StringType), types.IntType, | ||
| decls.OverloadExamples(`timestamp('2023-07-01T05:00:00Z').getDate('America/Los_Angeles') // 30`), | ||
| decls.BinaryBinding(timestampGetDayOfMonthOneBased))), | ||
|
|
||
| function(overloads.TimeGetDayOfWeek, | ||
| decls.FunctionDocs(`get the 0-based day of the week from a timestamp, UTC unless an IANA timezone is specified.`), | ||
| decls.MemberOverload(overloads.TimestampToDayOfWeek, | ||
| argTypes(types.TimestampType), types.IntType), | ||
| argTypes(types.TimestampType), types.IntType, | ||
| decls.OverloadExamples(`timestamp('2023-07-14T10:30:45.123Z').getDayOfWeek() // 5`), // Friday is day 5 | ||
| decls.UnaryBinding(func(ts ref.Val) ref.Val { | ||
| return timestampGetDayOfWeek(ts, utcTZ) | ||
| })), | ||
| decls.MemberOverload(overloads.TimestampToDayOfWeekWithTz, | ||
| argTypes(types.TimestampType, types.StringType), types.IntType)), | ||
| argTypes(types.TimestampType, types.StringType), types.IntType, | ||
| decls.OverloadExamples(`timestamp('2023-07-16T05:00:00Z').getDayOfWeek('America/Los_Angeles') // 6`), // Saturday is day 6 | ||
| decls.BinaryBinding(timestampGetDayOfWeek))), | ||
|
|
||
| function(overloads.TimeGetHours, | ||
| decls.FunctionDocs(`get the hours portion from a timestamp, or convert a duration to hours`), | ||
| decls.MemberOverload(overloads.TimestampToHours, | ||
| argTypes(types.TimestampType), types.IntType), | ||
| argTypes(types.TimestampType), types.IntType, | ||
| decls.OverloadExamples(`timestamp('2023-07-14T10:30:45.123Z').getHours() // 10`), | ||
| decls.UnaryBinding(func(ts ref.Val) ref.Val { | ||
| return timestampGetHours(ts, utcTZ) | ||
| })), | ||
| decls.MemberOverload(overloads.TimestampToHoursWithTz, | ||
| argTypes(types.TimestampType, types.StringType), types.IntType), | ||
| argTypes(types.TimestampType, types.StringType), types.IntType, | ||
| decls.OverloadExamples(`timestamp('2023-07-14T10:30:45.123Z').getHours('America/Los_Angeles') // 2`), | ||
| decls.BinaryBinding(timestampGetHours)), | ||
| decls.MemberOverload(overloads.DurationToHours, | ||
| argTypes(types.DurationType), types.IntType)), | ||
| argTypes(types.DurationType), types.IntType, | ||
| decls.OverloadExamples(`duration('3723s').getHours() // 1`), | ||
| decls.UnaryBinding(types.DurationGetHours))), | ||
|
|
||
| function(overloads.TimeGetMinutes, | ||
| decls.FunctionDocs(`get the minutes portion from a timestamp, or convert a duration to minutes`), | ||
| decls.MemberOverload(overloads.TimestampToMinutes, | ||
| argTypes(types.TimestampType), types.IntType), | ||
| argTypes(types.TimestampType), types.IntType, | ||
| decls.OverloadExamples(`timestamp('2023-07-14T10:30:45.123Z').getMinutes() // 30`), | ||
| decls.UnaryBinding(func(ts ref.Val) ref.Val { | ||
| return timestampGetMinutes(ts, utcTZ) | ||
| })), | ||
| decls.MemberOverload(overloads.TimestampToMinutesWithTz, | ||
| argTypes(types.TimestampType, types.StringType), types.IntType), | ||
| argTypes(types.TimestampType, types.StringType), types.IntType, | ||
| decls.OverloadExamples(`timestamp('2023-07-14T10:30:45.123Z').getMinutes('America/Los_Angeles') // 30`), | ||
| decls.BinaryBinding(timestampGetMinutes)), | ||
| decls.MemberOverload(overloads.DurationToMinutes, | ||
| argTypes(types.DurationType), types.IntType)), | ||
| argTypes(types.DurationType), types.IntType, | ||
| decls.OverloadExamples(`duration('3723s').getMinutes() // 62`), | ||
| decls.UnaryBinding(types.DurationGetMinutes))), | ||
|
|
||
| function(overloads.TimeGetSeconds, | ||
| decls.FunctionDocs(`get the seconds portion from a timestamp, or convert a duration to seconds`), | ||
| decls.MemberOverload(overloads.TimestampToSeconds, | ||
| argTypes(types.TimestampType), types.IntType), | ||
| argTypes(types.TimestampType), types.IntType, | ||
| decls.OverloadExamples(`timestamp('2023-07-14T10:30:45.123Z').getSeconds() // 45`), | ||
| decls.UnaryBinding(func(ts ref.Val) ref.Val { | ||
| return timestampGetSeconds(ts, utcTZ) | ||
| })), | ||
| decls.MemberOverload(overloads.TimestampToSecondsWithTz, | ||
| argTypes(types.TimestampType, types.StringType), types.IntType), | ||
| argTypes(types.TimestampType, types.StringType), types.IntType, | ||
| decls.OverloadExamples(`timestamp('2023-07-14T10:30:45.123Z').getSeconds('America/Los_Angeles') // 45`), | ||
| decls.BinaryBinding(timestampGetSeconds)), | ||
| decls.MemberOverload(overloads.DurationToSeconds, | ||
| argTypes(types.DurationType), types.IntType)), | ||
| argTypes(types.DurationType), types.IntType, | ||
| decls.OverloadExamples(`duration('3723.456s').getSeconds() // 3723`), | ||
| decls.UnaryBinding(types.DurationGetSeconds))), | ||
|
|
||
| function(overloads.TimeGetMilliseconds, | ||
| decls.FunctionDocs(`get the milliseconds portion from a timestamp`), | ||
| decls.MemberOverload(overloads.TimestampToMilliseconds, | ||
| argTypes(types.TimestampType), types.IntType), | ||
| argTypes(types.TimestampType), types.IntType, | ||
| decls.OverloadExamples(`timestamp('2023-07-14T10:30:45.123Z').getMilliseconds() // 123`), | ||
| decls.UnaryBinding(func(ts ref.Val) ref.Val { | ||
| return timestampGetMilliseconds(ts, utcTZ) | ||
| })), | ||
| decls.MemberOverload(overloads.TimestampToMillisecondsWithTz, | ||
| argTypes(types.TimestampType, types.StringType), types.IntType), | ||
| argTypes(types.TimestampType, types.StringType), types.IntType, | ||
| decls.OverloadExamples(`timestamp('2023-07-14T10:30:45.123Z').getMilliseconds('America/Los_Angeles') // 123`), | ||
| decls.BinaryBinding(timestampGetMilliseconds)), | ||
| decls.MemberOverload(overloads.DurationToMilliseconds, | ||
| argTypes(types.DurationType), types.IntType)), | ||
| argTypes(types.DurationType), types.IntType, | ||
| decls.UnaryBinding(types.DurationGetMilliseconds))), | ||
| } |
There was a problem hiding this comment.
getFullYear doc wording is misleading (year is not 0‑based)
TimeGetFullYear returns t.Year() (calendar year), but the doc string says “get the 0-based full year…”, which is confusing since years are not 0-based. The other time docs (0-based month/day-of-year/day-of-month vs 1-based date) correctly match their implementations.
Consider updating the doc to drop “0-based”, e.g.:
- decls.FunctionDocs(`get the 0-based full year from a timestamp, UTC unless an IANA timezone is specified.`),
+ decls.FunctionDocs(`get the full year from a timestamp, UTC unless an IANA timezone is specified.`),🤖 Prompt for AI Agents
vendor/github.com/google/cel-go/common/stdlib/standard.go around lines 743 to
888: the FunctionDocs for TimeGetFullYear incorrectly says "0-based full year"
while the implementation returns the calendar year (t.Year()); update the doc
string to remove "0-based" — e.g. change it to "get the full year from a
timestamp, UTC unless an IANA timezone is specified." to match the behavior and
be consistent with other time docs.
| // partialActivationConverter indicates whether an Activation implementation supports conversion to a PartialActivation | ||
| type partialActivationConverter interface { | ||
| asPartialActivation() (PartialActivation, bool) | ||
| // AsPartialActivation converts the current activation to a PartialActivation | ||
| AsPartialActivation() (PartialActivation, bool) | ||
| } |
There was a problem hiding this comment.
Guard AsPartialActivation against nil Activation to avoid panics
AsPartialActivation(vars Activation) will panic if called with a nil vars value, since vars.Parent() is invoked unconditionally when the type assertion fails. As an exported helper, it’s safer to treat nil as “no partial activation”.
A minimal defensive tweak:
func AsPartialActivation(vars Activation) (PartialActivation, bool) {
- // Only internal activation instances may implement this interface
- if pv, ok := vars.(partialActivationConverter); ok {
+ if vars == nil {
+ return nil, false
+ }
+ // Only internal activation instances may implement this interface
+ if pv, ok := vars.(partialActivationConverter); ok {
return pv.AsPartialActivation()
}
// Since Activations may be hierarchical, test whether a parent converts to a PartialActivation
- if vars.Parent() != nil {
- return AsPartialActivation(vars.Parent())
- }
- return nil, false
+ if parent := vars.Parent(); parent != nil {
+ return AsPartialActivation(parent)
+ }
+ return nil, false
}Also applies to: 176-179, 181-191
🤖 Prompt for AI Agents
In vendor/github.com/google/cel-go/interpreter/activation.go around lines
159-163 (and similarly for the related code at 176-179 and 181-191), the
AsPartialActivation helper assumes the passed-in Activation is non-nil and may
call vars.Parent() leading to a panic; update the function to first check if
vars == nil and immediately return (nil, false) when it is nil, and apply the
same nil guard before any Parent() calls or type assertions in the other
referenced blocks so nil Activation is treated as “no partial activation.”
6s Warning FailedDescribeInstanceTypes awscluster/ngirard-dh-5bb5w insufficient permissions to describe instance types for instance type "m6i.xlarge", falling back to the default architecture of "x86_64": operation error EC2: DescribeInstanceTypes, https response error StatusCode: 403, RequestID: 387549b4-ab58-48af-b14d-3882b6c7da52, api error UnauthorizedOperation: You are not authorized to perform this operation. User: arn:aws:iam::726924432237:user/ngirard-dh-5bb5w-openshift-cluster-api-aws-72f7q is not authorized to perform: ec2:DescribeInstanceTypes because no identity-based policy allows the ec2:DescribeInstanceTypes action
|
@vr4manta: This pull request references SPLAT-2167 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/conversion/capi2mapi/aws_fuzz_test.go (1)
211-221: Consider explicitly clearing HostID for non-host affinity cases.When
HostAffinityisnilor"default", theHostIDshould be explicitly set tonilto prevent test pollution from the earlierc.FillNoCustom(spec)call at line 158, which may have set random values.Apply this diff:
func fuzzAWSMachineSpecHostPlacement(spec *awsv1.AWSMachineSpec, c randfill.Continue) { switch c.Int31n(3) { case 0: spec.HostAffinity = nil + spec.HostID = nil case 1: spec.HostAffinity = ptr.To("default") + spec.HostID = nil case 2: spec.HostAffinity = ptr.To("host") spec.HostID = ptr.To("h-0123456789abcdef0") } }
📜 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 (12)
manifests/0000_30_cluster-api_01_credentials-request.yaml(1 hunks)pkg/conversion/capi2mapi/aws.go(3 hunks)pkg/conversion/capi2mapi/aws_fuzz_test.go(3 hunks)pkg/conversion/capi2mapi/aws_test.go(6 hunks)pkg/conversion/capi2mapi/machine_test.go(1 hunks)pkg/conversion/capi2mapi/openstack_test.go(6 hunks)pkg/conversion/capi2mapi/powervs_test.go(3 hunks)pkg/conversion/mapi2capi/aws.go(2 hunks)pkg/conversion/mapi2capi/aws_fuzz_test.go(3 hunks)pkg/conversion/mapi2capi/aws_test.go(8 hunks)pkg/conversion/mapi2capi/machine_test.go(1 hunks)pkg/conversion/mapi2capi/powervs_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/conversion/capi2mapi/openstack_test.go
- pkg/conversion/mapi2capi/machine_test.go
- pkg/conversion/capi2mapi/aws.go
- manifests/0000_30_cluster-api_01_credentials-request.yaml
- pkg/conversion/capi2mapi/aws_test.go
- pkg/conversion/mapi2capi/aws.go
🔇 Additional comments (13)
pkg/conversion/capi2mapi/machine_test.go (1)
75-89: LGTM! Improved error message readability.The test expectations have been correctly updated to reflect improved duration formatting in error messages. The change from verbose duration representations to the standard Go duration string format
"1s"enhances readability while maintaining test correctness.pkg/conversion/mapi2capi/powervs_test.go (2)
124-124: LGTM! Error message formatting improved.The error expectation now uses JSON formatting instead of Go struct string representation, improving readability and consistency with standard error message formats.
139-139: LGTM! Consistent error formatting improvement.The metadata error expectation now uses concise JSON formatting, consistent with the loadBalancers change on Line 124.
pkg/conversion/capi2mapi/powervs_test.go (3)
100-100: LGTM! Correct null representation.Removing quotes around
nullis semantically correct—null is not a string literal and should appear unquoted in error messages.
125-125: LGTM! Consistent null formatting.This change matches the correction on Line 100, ensuring consistent null representation across error expectations.
150-150: LGTM! Simplified empty struct representation.The error expectation now uses
{}instead of a verbose Go struct representation, providing a cleaner and more concise format for the empty struct.pkg/conversion/capi2mapi/aws_fuzz_test.go (2)
32-32: LGTM! Import needed for fuzzing.The
k8s.io/utils/ptrimport is correctly added to support pointer creation in the newfuzzAWSMachineSpecHostPlacementfunction.
162-162: LGTM! Fuzzing hook properly integrated.The host placement fuzzing is correctly integrated into the
AWSMachineSpecfuzzing flow.pkg/conversion/mapi2capi/aws_fuzz_test.go (3)
140-140: LGTM! Appropriate field clearing.Clearing
CPUOptionsis consistent with other unsupported fields being cleared in this function. This appears to address test failures mentioned in the PR description related to upstream updates.
167-180: LGTM! Host placement fuzzing correctly implemented.The fuzzer appropriately covers the two valid
HostPlacementstates:AnyAvailable(withDedicatedHostcleared) andDedicatedHost(with a valid ID). The explicit clearing ofDedicatedHostfor theAnyAvailablecase prevents invalid state combinations.
276-276: LGTM! Fuzzer properly registered.The
fuzzHostPlacementfunction is correctly added to the fuzzer function list, ensuringHostPlacementfields are appropriately fuzzed during machine set conversion testing.pkg/conversion/mapi2capi/aws_test.go (2)
132-132: LGTM! Improved error message formatting.The error messages have been updated to use JSON-like formatting, making them more structured and easier to parse. This improves test readability and better matches the actual error output format.
Also applies to: 211-211, 246-246, 258-258, 348-348, 394-394
429-429: LGTM! Helpful documentation comment.The comment clearly documents that dedicated host tests were relocated into the
DescribeTableabove, making the test reorganization transparent to future maintainers.
| // Dedicated host positive/negative cases | ||
| Entry("With HostPlacement AnyAvailable", awsMAPI2CAPIConversionInput{ | ||
| machineBuilder: awsMAPIMachineBase.WithProviderSpecBuilder( | ||
| awsBaseProviderSpec. | ||
| WithHostPlacement(&mapiv1beta1.HostPlacement{ | ||
| Affinity: ptr.To(mapiv1beta1.HostAffinityAnyAvailable), | ||
| }), | ||
| ), | ||
| infra: infra, | ||
| expectedErrors: []string{}, | ||
| expectedWarnings: []string{}, | ||
| }), | ||
| Entry("With HostPlacement DedicatedHost", awsMAPI2CAPIConversionInput{ | ||
| machineBuilder: awsMAPIMachineBase.WithProviderSpecBuilder( | ||
| awsBaseProviderSpec. | ||
| WithHostPlacement(&mapiv1beta1.HostPlacement{ | ||
| Affinity: ptr.To(mapiv1beta1.HostAffinityDedicatedHost), | ||
| DedicatedHost: &mapiv1beta1.DedicatedHost{ | ||
| ID: "h-0123456789abcdef0", | ||
| }, | ||
| }), | ||
| ), | ||
| infra: infra, | ||
| expectedErrors: []string{}, | ||
| expectedWarnings: []string{}, | ||
| }), |
There was a problem hiding this comment.
🧩 Analysis chain
LGTM! Positive test cases well covered.
The two new test entries appropriately cover the valid HostPlacement scenarios: AnyAvailable and DedicatedHost with a valid ID. Both tests correctly expect successful conversion with no errors or warnings.
The comment at line 96 mentions "positive/negative cases," but only positive cases are present. Consider verifying that negative test cases (e.g., invalid HostID format, missing DedicatedHost ID when affinity is DedicatedHost) are covered elsewhere in the test suite or in the conversion validation logic.
🏁 Script executed:
#!/bin/bash
# Search for negative test cases or validation logic for HostPlacement
rg -n -C3 --type=go 'HostPlacement|HostAffinity|DedicatedHost' -g '*test*.go' -g '!vendor/'Length of output: 7234
I'll search for the conversion logic to understand if negative test cases should exist for the mapi2capi direction.
🏁 Script executed:
#!/bin/bash
# Find the conversion logic for HostPlacement in mapi2capi
rg -n 'HostPlacement|HostAffinity' --type=go -g '!*test*.go' -g '!vendor/' pkg/conversion/mapi2capi/Length of output: 65
🏁 Script executed:
#!/bin/bash
# Find conversion logic for HostPlacement
fd -e '.go' -path '*/mapi2capi/*' -not -path '*/vendor/*' -not -name '*test*' | head -20Length of output: 168
🏁 Script executed:
#!/bin/bash
# Search for HostPlacement conversion logic across mapi2capi
rg -n 'func.*Convert|HostPlacement|HostAffinity' --type=go -g '!*test*' -g '!vendor/' 2>/dev/null | head -50Length of output: 1781
🏁 Script executed:
#!/bin/bash
# Get the full implementation of the mapi2capi conversion functions
rg -n -A 15 'func convertAWSHostAffinityToCAPI|func convertAWSHostIDToCAPI' --type=go pkg/conversion/mapi2capi/aws.goLength of output: 1104
Update comment at line 96 to reflect actual test coverage.
The comment states "Dedicated host positive/negative cases" but the code only contains positive test cases. This is correct by design—the mapi2capi conversion logic silently converts or returns nil for unknown values (no validation errors). Unlike the reverse direction (capi2mapi), which validates and rejects unsupported values, negative test cases don't apply here.
Update line 96 comment from:
// Dedicated host positive/negative cases
to:
// Dedicated host cases
🤖 Prompt for AI Agents
In pkg/conversion/mapi2capi/aws_test.go around lines 96 to 121 the comment "//
Dedicated host positive/negative cases" is inaccurate because the tests only
cover positive cases; change the comment to "// Dedicated host cases" to reflect
actual test coverage; update that single-line comment accordingly without
modifying test logic.
|
/retest |
|
/lgtm |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nrb 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
|
/retest |
|
/verified by @vr4manta by manual tests |
openshift-ci-robot
added
the
verified
|
/retest |
|
@vr4manta: 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. |
|
/retest |
|
@vr4manta: The following tests failed, say
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. |
|
It seems the openstack techpreview have been having issues for a while:
@damdo Is there any known issues with these two jobs? If so, is it possible to override them here so we can get this merged? I am not wanting to keep retrying because TRT may assume that w/ all the failures, this PR may be the cause (which is why I am putting the above links to show other PRs hit same install issue this PR is) |
6 participants
SPLAT-2167
Changes
ec2:DescribeInstanceTypesto cluster api credentials requestDependencies
Notes
There seems to be a required permission that was missing for dedicated host support. When running these changes, a warning event was observed in the cluster capi operator namespace.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests