PHP 8.2+ Modernization, Advanced Authentication, and CSI Storage Support #489
Conversation
Reviewer's GuideModernizes the library for PHP 8.2+ with enums and strict typing, introduces a pluggable token-provider–based authentication system (ExecCredential, EKS, OpenShift OAuth, and ServiceAccount TokenRequest), adds first-class CSI storage resources and tests, and updates CI, docs, and style accordingly. Sequence diagram for request authentication using token providerssequenceDiagram
actor Dev
participant KubernetesCluster
participant AuthTrait as AuthenticatesCluster
participant Provider as TokenProvider
participant HTTP as HttpClient
participant IdP as ExternalIdentity
Dev->>KubernetesCluster: new KubernetesCluster(url)
Dev->>AuthTrait: withOpenShiftAuth(username, password)
AuthTrait->>Provider: new OpenShiftOAuthProvider(url, username, password)
AuthTrait->>AuthTrait: withTokenProvider(Provider)
Dev->>KubernetesCluster: someResource->get()
KubernetesCluster->>AuthTrait: getAuthToken()
AuthTrait->>Provider: getToken()
alt token missing or expired
Provider->>IdP: refresh() (OAuth/EKS/Exec/TokenRequest)
IdP-->>Provider: new token + expiration
end
Provider-->>AuthTrait: token
AuthTrait-->>KubernetesCluster: token
KubernetesCluster->>HTTP: getClient() with Authorization: Bearer token
HTTP-->>KubernetesCluster: HTTP response
KubernetesCluster-->>Dev: resource result
Class diagram for token provider–based authentication architectureclassDiagram
direction LR
class KubernetesCluster {
-?string url
-?string resourceClass
-?string token
-TokenProviderInterface tokenProvider
+__construct(?string url)
+setResourceClass(string resourceClass) static
+runOperation(Operation operation, string path, string|null|Closure payload, array query) mixed
+getAuthToken() ?string
+call(string method, string path, string payload) mixed
}
class AuthenticatesCluster {
-?string token
-TokenProviderInterface tokenProvider
+withToken(?string token) self
+withTokenProvider(TokenProviderInterface provider) self
+withEksAuth(string clusterName, string region) self
+withOpenShiftAuth(string username, string password) self
+withServiceAccountToken(string namespace, string serviceAccount, int expirationSeconds, ?array audiences) self
+getAuthToken() ?string
+inClusterConfiguration(string url) KubernetesCluster
}
class TokenProviderInterface {
<<interface>>
+getToken() string
+isExpired() bool
+refresh() void
+getExpiresAt() DateTimeInterface
}
class TokenProvider {
<<abstract>>
-?string token
-?DateTimeInterface expiresAt
-int refreshBuffer
+getToken() string
+isExpired() bool
+getExpiresAt() ?DateTimeInterface
+setRefreshBuffer(int seconds) self
+refresh() void*
}
class ExecCredentialProvider {
-string command
-array args
-array env
-string apiVersion
-?string installHint
-bool provideClusterInfo
-?array clusterInfo
+__construct(array execConfig)
+setClusterInfo(array clusterInfo) self
+refresh() void
}
class EksTokenProvider {
-string clusterName
-string region
-?string roleArn
-?string profile
-int tokenTtl
+__construct(string clusterName, string region)
+withProfile(string profile) self
+withAssumeRole(string roleArn) self
+refresh() void
+getCredentials() Aws~CredentialsInterface~
+isAvailable() bool$
}
class OpenShiftOAuthProvider {
-string clusterUrl
-string username
-string password
-?string oauthEndpoint
-bool verifySsl
-int defaultTokenTtl
+__construct(string clusterUrl, string username, string password)
+withOAuthEndpoint(string endpoint) self
+withoutSslVerification() self
+refresh() void
-discoverOAuthEndpoint() string
}
class ServiceAccountTokenProvider {
-KubernetesCluster bootstrapCluster
-string namespace
-string serviceAccount
-int expirationSeconds
-array audiences
-?string bootstrapToken
+__construct(KubernetesCluster bootstrapCluster, string namespace, string serviceAccount)
+withExpirationSeconds(int seconds) self
+withAudiences(array audiences) self
+refresh() void
-makeBootstrapRequest(string method, string path, string payload) mixed
}
class MakesHttpCalls {
+getClient() GuzzleHttp~Client~
}
class MakesWebsocketCalls {
+getWsClient(string url) array
+buildStreamContextOptions() array
-createSocketConnection(string callableUrl) resource|false
}
class AuthenticationException {
}
KubernetesCluster ..|> AuthenticatesCluster : uses
KubernetesCluster ..|> MakesHttpCalls : uses
KubernetesCluster ..|> MakesWebsocketCalls : uses
AuthenticatesCluster ..> TokenProviderInterface : has
MakesHttpCalls ..> AuthenticatesCluster : getAuthToken
MakesWebsocketCalls ..> AuthenticatesCluster : getAuthToken
TokenProviderInterface <|.. TokenProvider
TokenProvider <|-- ExecCredentialProvider
TokenProvider <|-- EksTokenProvider
TokenProvider <|-- OpenShiftOAuthProvider
TokenProvider <|-- ServiceAccountTokenProvider
ExecCredentialProvider ..> AuthenticationException
EksTokenProvider ..> AuthenticationException
OpenShiftOAuthProvider ..> AuthenticationException
ServiceAccountTokenProvider ..> AuthenticationException
ServiceAccountTokenProvider ..> KubernetesCluster : bootstrapCluster
Class diagram for new CSI storage resource kindsclassDiagram
direction LR
class K8sResource {
+setSpec(string key, mixed value) self
+getSpec(string key, mixed default) mixed
+setAttribute(string key, mixed value) self
+getAttribute(string key, mixed default) mixed
}
class InteractsWithK8sCluster {
<<interface>>
}
class Watchable {
<<interface>>
}
class HasSpec {
+setSpec(string key, mixed value) self
+getSpec(string key, mixed default) mixed
}
class K8sCSIDriver {
<<cluster-scoped>>
+static string kind = "CSIDriver"
+static string defaultVersion = "storage.k8s.io/v1"
+static bool namespaceable = false
+setAttachRequired(bool required) self
+isAttachRequired() bool
+setPodInfoOnMount(bool podInfo) self
+isPodInfoOnMount() bool
+setVolumeLifecycleModes(array modes) self
+getVolumeLifecycleModes() array
+setStorageCapacity(bool capacity) self
+hasStorageCapacity() bool
+setFsGroupPolicy(string policy) self
+getFsGroupPolicy() ?string
+setTokenRequests(array requests) self
+getTokenRequests() array
+setRequiresRepublish(bool requires) self
+requiresRepublish() bool
+setSELinuxMount(bool selinux) self
+hasSELinuxMount() bool
}
class K8sCSINode {
<<cluster-scoped>>
+static string kind = "CSINode"
+static string defaultVersion = "storage.k8s.io/v1"
+static bool namespaceable = false
+getDrivers() array
+getDriverByName(string driverName) ?array
+hasDriver(string driverName) bool
+getNodeIdForDriver(string driverName) ?string
+getTopologyKeysForDriver(string driverName) array
+getAllocatableForDriver(string driverName) ?array
}
class K8sVolumeAttributesClass {
<<cluster-scoped>>
+static string kind = "VolumeAttributesClass"
+static string defaultVersion = "storage.k8s.io/v1"
+static bool namespaceable = false
+setDriverName(string driver) self
+getDriverName() ?string
+setParameters(array parameters) self
+getParameters() array
}
class InitializesResources {
+csiDriver(mixed cluster, array attributes) K8sCSIDriver$
+csiNode(mixed cluster, array attributes) K8sCSINode$
+volumeAttributesClass(mixed cluster, array attributes) K8sVolumeAttributesClass$
}
K8sResource <|-- K8sCSIDriver
K8sResource <|-- K8sCSINode
K8sResource <|-- K8sVolumeAttributesClass
K8sCSIDriver ..|> InteractsWithK8sCluster
K8sCSIDriver ..|> Watchable
K8sCSIDriver ..|> HasSpec
K8sCSINode ..|> InteractsWithK8sCluster
K8sCSINode ..|> Watchable
K8sCSINode ..|> HasSpec
K8sVolumeAttributesClass ..|> InteractsWithK8sCluster
K8sVolumeAttributesClass ..|> Watchable
InitializesResources ..> K8sCSIDriver : csiDriver()
InitializesResources ..> K8sCSINode : csiNode()
InitializesResources ..> K8sVolumeAttributesClass : volumeAttributesClass()
Class diagram for new enums and updated resource behaviorsclassDiagram
direction LR
class Operation {
<<enum>>
GET
CREATE
REPLACE
DELETE
LOG
WATCH
WATCH_LOGS
EXEC
ATTACH
APPLY
JSON_PATCH
JSON_MERGE_PATCH
+httpMethod() string
+usesWebSocket() bool
}
class PodPhase {
<<enum>>
PENDING
RUNNING
SUCCEEDED
FAILED
UNKNOWN
+isTerminal() bool
+isActive() bool
+isSuccessful() bool
}
class RestartPolicy {
<<enum>>
ALWAYS
ON_FAILURE
NEVER
+allowsRestarts() bool
+onlyOnFailure() bool
}
class ServiceType {
<<enum>>
CLUSTER_IP
NODE_PORT
LOAD_BALANCER
EXTERNAL_NAME
+isExternallyAccessible() bool
+createsLoadBalancer() bool
}
class Protocol {
<<enum>>
TCP
UDP
SCTP
+isConnectionOriented() bool
}
class ConditionStatus {
<<enum>>
TRUE
FALSE
UNKNOWN
+isTrue() bool
+isFalse() bool
+isKnown() bool
}
class AccessMode {
<<enum>>
READ_WRITE_ONCE
READ_ONLY_MANY
READ_WRITE_MANY
READ_WRITE_ONCE_POD
+allowsWrite() bool
+allowsMultiple() bool
+isPodScoped() bool
}
class PullPolicy {
<<enum>>
ALWAYS
NEVER
IF_NOT_PRESENT
+alwaysPulls() bool
+allowsCached() bool
}
class KubernetesCluster {
+runOperation(Operation|string operation, string path, string|null|Closure payload, array query) mixed
}
class RunsClusterOperations {
+all(array query) mixed
+allNamespaces(array query) mixed
+get(array query) mixed
+create(array query) mixed
+update(array query) bool
+delete(array query, mixed gracePeriod, string propagationPolicy) void
+apply(string fieldManager, bool force, array query) mixed
+jsonPatch(mixed patch, array query) mixed
+jsonMergePatch(mixed patch, array query) mixed
+watchAll(Closure callback, array query) mixed
+watch(Closure callback, array query) mixed
+logs(array query) mixed
+watchLogs(Closure callback, array query) mixed
+exec(array command, ?string container, array query) mixed
+attach(Closure callback, ?string container, array query) mixed
}
class K8sPod {
+restartOnFailure() static
+neverRestart() static
+setRestartPolicy(RestartPolicy policy) static
+getRestartPolicy() RestartPolicy
+getPodPhase() PodPhase
+isRunning() bool
+isSuccessful() bool
+isTerminal() bool
}
class K8sService {
+setType(ServiceType type) static
+getType() ServiceType
+isExternallyAccessible() bool
+getClusterDns() ?string
}
class K8sScale {
+create(array query) mixed
+update(array query) bool
}
KubernetesCluster ..> Operation : runOperation()
RunsClusterOperations ..> Operation : uses
K8sScale ..> Operation : uses
K8sPod ..> RestartPolicy : uses
K8sPod ..> PodPhase : uses
K8sService ..> ServiceType : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@cuppett has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 5 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (53)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a token provider system (interface, abstract, and EKS/Exec/OpenShift/ServiceAccount implementations), wires providers into cluster HTTP/WS and kubeconfig exec handling, introduces multiple enums and CSI/volume kinds, tightens return types, updates CI/composer, adds docs/tests, and removes patch docs/examples. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Cluster as KubernetesCluster
participant Provider as TokenProvider
participant External as ExternalAuthSystem
rect rgb(235,242,255)
Client->>Cluster: Make API request
Cluster->>Provider: getAuthToken()
end
alt provider has valid cached token
Provider-->>Cluster: return token
else provider missing/expired
rect rgb(240,255,240)
Cluster->>Provider: refresh()
Provider->>External: fetch token (STS / OAuth / exec / TokenRequest)
External-->>Provider: token + expiration
Provider->>Provider: store token & expiresAt
Provider-->>Cluster: return new token
end
end
rect rgb(255,250,235)
Cluster->>External: API call with Authorization: Bearer token
External-->>Cluster: response
Cluster-->>Client: return response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @cuppett, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly modernizes the php-k8s client by upgrading its PHP version compatibility to 8.2+ and integrating contemporary language features like enums and advanced type hinting. It also bolsters the client's security and flexibility by introducing a suite of advanced authentication providers, enabling seamless integration with various Kubernetes environments such as OpenShift and EKS. Furthermore, the PR expands the client's capabilities to manage CSI storage resources, providing a more comprehensive and robust solution for Kubernetes interactions. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The change to
.github/dependabot.ymlremoves the top-levelupdates:key and leaves a bare list, which is not valid Dependabot v2 syntax and will likely disable updates—consider restoring theupdates:key and nesting the items under it. - In
AuthenticatesCluster::withTokenFromCommandProviderthe docblock has a typo in the@paramannotation (string|nll), which should be corrected tostring|nullto keep tooling and IDEs accurate. - The new
ServiceAccountTokenProviderrelies on reflection to manipulateKubernetesCluster's privatetokenandtokenProviderproperties; it would be more robust to add a small internal API onKubernetesClusterto perform this bootstrap call instead of reaching into private state.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The change to `.github/dependabot.yml` removes the top-level `updates:` key and leaves a bare list, which is not valid Dependabot v2 syntax and will likely disable updates—consider restoring the `updates:` key and nesting the items under it.
- In `AuthenticatesCluster::withTokenFromCommandProvider` the docblock has a typo in the `@param` annotation (`string|nll`), which should be corrected to `string|null` to keep tooling and IDEs accurate.
- The new `ServiceAccountTokenProvider` relies on reflection to manipulate `KubernetesCluster`'s private `token` and `tokenProvider` properties; it would be more robust to add a small internal API on `KubernetesCluster` to perform this bootstrap call instead of reaching into private state.
## Individual Comments
### Comment 1
<location> `src/Traits/Cluster/AuthenticatesCluster.php:81-86` </location>
<code_context>
/**
* Load the token from provider command line.
*
- * @param string|null $cmdArgs
+ * @param string|nll $cmdArgs
* @return $this
*/
</code_context>
<issue_to_address>
**issue (typo):** Fix `@param` phpdoc type typo (`string|nll` → `string|null`).
The phpdoc type for `$cmdArgs` should be `string|null` (matching `?string`), not `string|nll`, to avoid confusion and potential issues with static analysis tools.
```suggestion
/**
* Load the token from provider command line.
*
+ * @param string|null $cmdArgs
* @return $this
*/
```
</issue_to_address>
### Comment 2
<location> `.github/dependabot.yml:2-8` </location>
<code_context>
version: 2
updates:
- - package-ecosystem: github-actions
- directory: "/"
- schedule:
- interval: weekly
- open-pull-requests-limit: 10
- - package-ecosystem: composer
- directory: "/"
- schedule:
</code_context>
<issue_to_address>
**issue (bug_risk):** The `updates` key was removed from dependabot config, which breaks the YAML structure.
In this change the list items were moved to the top level under `version: 2`, removing the required `updates:` key. Dependabot requires the `updates` mapping, otherwise it will ignore the config. Please restore the original structure with `updates:` containing the list of ecosystems.
</issue_to_address>
### Comment 3
<location> `src/KubernetesCluster.php:171-173` </location>
<code_context>
+ public function runOperation(Operation|string $operation, string $path, string|null|Closure $payload = '', array $query = ['pretty' => 1]): mixed
</code_context>
<issue_to_address>
**issue (bug_risk):** Converting string operations via `Operation::from()` changes behavior for unknown operations (now throws).
Previously, unknown `$operation` strings fell back to `GET`. Using `Operation::from($operation)` now throws `ValueError` for those cases, which can break callers that send custom/legacy operation strings. If backward compatibility for unknown strings is required, use `tryFrom()` with a fallback, e.g.:
```php
if (is_string($operation)) {
$operation = Operation::tryFrom($operation) ?? Operation::GET;
}
```
rather than `from()`.
</issue_to_address>
### Comment 4
<location> `AGENTS.md:106` </location>
<code_context>
+- **State tracking:** `isSynced()` - resource has been synced with cluster; `exists()` - resource currently exists in cluster.
+
+**Common Tasks**
+- **Add a new builtin kind:**
+ 1. Create `src/Kinds/K8sYourKind.php` extending `K8sResource`:
+ ```php
</code_context>
<issue_to_address>
**suggestion (typo):** Consider changing "builtin" to the more standard "built-in" in this heading.
For example, change the heading text to: `Add a new built-in kind:`.
```suggestion
- **Add a new built-in kind:**
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a substantial and well-executed pull request that modernizes the library to PHP 8.2+ standards, introduces a robust and extensible authentication provider system, and adds support for CSI storage resources. The use of enums and match expressions significantly improves code clarity and type safety. The new authentication providers for EKS, OpenShift, Exec plugins, and Service Account tokens are great additions and appear to be implemented correctly, including token refresh logic. The test coverage for these new features is comprehensive.
My review includes a few minor suggestions, mostly related to documentation consistency and a small improvement for the EKS token provider to ensure unique session names. Overall, this is an excellent contribution.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Kinds/K8sResource.php (1)
153-164: Handle potential false return from toJson().Line 155 calls
toJson()which can now explicitly returnfalseon encoding failure. The subsequentstr_replace()calls don't check for this, meaning if JSON encoding fails, the method will perform string replacements onfalse(which PHP coerces to an empty string), resulting in an empty or invalid payload being returned.While
json_encode()rarely fails in practice, when it does (circular references, encoding errors, deeply nested structures), this will cause silent failures that are difficult to debug.🔎 Suggested fix to handle encoding failure
public function toJsonPayload(?string $kind = null) { $attributes = $this->toJson(JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES, $kind); + + if ($attributes === false) { + throw new \RuntimeException('Failed to encode resource to JSON'); + } $attributes = str_replace(': []', ': {}', $attributes);
🧹 Nitpick comments (16)
src/Kinds/K8sVerticalPodAutoscaler.php (1)
40-47: Consider adding return type for consistency.The
setTargetmethod lacks a declared return type (though the docblock indicates@return $this). For full consistency with the modernization effort, consider adding: statichere as well.🔎 Suggested addition
- public function setTarget(string $apiVersion, string $kind, string $name) + public function setTarget(string $apiVersion, string $kind, string $name): static.github/workflows/ci.yml (1)
34-63: PHP 8.5 matrix entries are verbose but functional.You've added six explicit combinations for PHP 8.5 instead of using the matrix cross-product with excludes. This works and makes the PHP 8.5/Laravel 12.* restriction crystal clear, but it's repetitive. If you add another Kubernetes version later, you'll need to remember to add two more entries here.
Alternative: Use matrix cross-product with excludes
You could reduce duplication by including 8.5 in the main php array and excluding unwanted combinations:
matrix: - php: ['8.2', '8.3', '8.4'] + php: ['8.2', '8.3', '8.4', '8.5'] kubernetes: ['1.32.9', '1.33.5', '1.34.1'] laravel: ['11.*', '12.*'] prefer: [prefer-lowest, prefer-stable] include: - laravel: "11.*" testbench: "9.*" - laravel: "12.*" testbench: "10.*" - # PHP 8.5 only for Laravel 12 - - php: '8.5' - kubernetes: '1.32.9' - laravel: '12.*' - testbench: "10.*" - prefer: prefer-lowest - ... + exclude: + - php: '8.5' + laravel: '11.*'But your current approach is explicit and perfectly fine. Up to you.
tests/Auth/OpenShiftOAuthProviderTest.php (2)
21-38: Test makes real HTTP requests—will be slow and flaky.
test_oauth_endpoint_discovery_patterninvokesdiscoverOAuthEndpoint(), which attempts an actual HTTP GET tohttps://api.cluster.example.com:6443/.well-known/oauth-authorization-server. This will:
- Hang/timeout if DNS resolves and connection is attempted
- Fail fast if DNS fails (depends on resolver behavior)
- Be non-deterministic across environments
Mock the HTTP client or inject a test double. Otherwise, this test will be slow (~5-30s timeout) and potentially flaky in CI.
58-105: Reflection-heavy tests are brittle.Every test here accesses protected properties/methods via reflection. If you rename
verifySsl,oauthEndpoint, ordefaultTokenTtl, these tests break silently at runtime rather than at type-check time.Consider either:
- Adding public getter methods for testability (if they make sense in the API)
- Using a test subclass that exposes internals
Not a blocker, but this will bite you during refactoring.
tests/CSIDriverTest.php (2)
149-153: Unused parameter$typein watch callback.The
$typeparameter receives the event type (ADDED, MODIFIED, DELETED) but isn't used in the callback logic. If you don't need it, either remove it or prefix with underscore ($_type) to signal intentional non-use.🔎 Proposed fix
- $watch = $this->cluster->csiDriver()->watchAll(function ($type, $csiDriver) { + $watch = $this->cluster->csiDriver()->watchAll(function ($_type, $csiDriver) { if ($csiDriver->getName() === 'test-csi-driver.example.com') { return true; } }, ['timeoutSeconds' => 10]);
160-162: Unused parameter$typein watch callback.Same issue as the previous watch callback - the
$typeparameter is unused.🔎 Proposed fix
- $watch = $this->cluster->csiDriver()->watchByName('test-csi-driver.example.com', function ($type, $csiDriver) { + $watch = $this->cluster->csiDriver()->watchByName('test-csi-driver.example.com', function ($_type, $csiDriver) { return $csiDriver->getName() === 'test-csi-driver.example.com'; }, ['timeoutSeconds' => 10]);src/Auth/ExecCredentialProvider.php (2)
51-56: Shell execution security consideration.Using
Process::fromShellCommandline()executes the command through a shell, which introduces potential command injection risks if the command or arguments originate from untrusted sources. Whileescapeshellarg()is used on line 53, you're still subject to shell interpretation.Consider using
new Process([...])with an array of arguments instead, which bypasses the shell entirely and is more secure.🔎 Proposed fix
- $commandLine = $this->command; - if (! empty($this->args)) { - $commandLine .= ' '.implode(' ', array_map('escapeshellarg', $this->args)); - } - - $process = Process::fromShellCommandline($commandLine); + // Build command as array to avoid shell interpretation + $command = array_merge([$this->command], $this->args); + $process = new Process($command);
99-101: Verify timestamp format from untrusted input.Line 99 creates a
DateTimeImmutablefrom an external timestamp without validation. If the exec credential provider returns an invalid timestamp format, this will throw an uncaught exception that's not anAuthenticationException.🔎 Proposed fix
if (isset($credential['status']['expirationTimestamp'])) { - $this->expiresAt = new \DateTimeImmutable( - $credential['status']['expirationTimestamp'] - ); + try { + $this->expiresAt = new \DateTimeImmutable( + $credential['status']['expirationTimestamp'] + ); + } catch (\Exception $e) { + throw new AuthenticationException( + "Invalid expirationTimestamp format: {$e->getMessage()}" + ); + } } else {src/Auth/TokenProvider.php (1)
28-37: UseDateTimeImmutablefor consistency.Line 34 instantiates
new \DateTime(mutable) while$expiresAtis declared as?DateTimeInterfaceand concrete providers may useDateTimeImmutable. The comparison works but mixing mutable/immutable datetime types is inconsistent. Usenew \DateTimeImmutableinstead.🔎 Proposed fix
public function isExpired(): bool { if ($this->expiresAt === null) { return false; // No expiration known, assume valid } - $bufferTime = (new \DateTime)->modify("+{$this->refreshBuffer} seconds"); + $bufferTime = (new \DateTimeImmutable)->modify("+{$this->refreshBuffer} seconds"); return $this->expiresAt <= $bufferTime; }tests/VolumeAttributesClassTest.php (1)
154-154: Prefix unused callback parameters with underscore.The
$typeparameter in both watch callbacks is unused but likely required by the callback signature. Use$_typeinstead to indicate the parameter is intentionally ignored, following PHP conventions.🔎 Proposed fix
- public function runWatchAllTests() - { - $watch = $this->cluster->volumeAttributesClass()->watchAll(function ($type, $vac) { + $watch = $this->cluster->volumeAttributesClass()->watchAll(function ($_type, $vac) { if ($vac->getName() === 'silver') { return true; } }, ['timeoutSeconds' => 10]); - public function runWatchTests() - { - $watch = $this->cluster->volumeAttributesClass()->watchByName('silver', function ($type, $vac) { + $watch = $this->cluster->volumeAttributesClass()->watchByName('silver', function ($_type, $vac) { return $vac->getName() === 'silver'; }, ['timeoutSeconds' => 10]);Also applies to: 165-165
tests/Auth/ExecCredentialIntegrationTest.php (1)
104-105: Consider command portability.The
printenvcommand may not be available on Windows systems. If cross-platform testing is a concern, consider using a PHP-based approach or documenting this Unix/Linux-only test requirement.src/Auth/EksTokenProvider.php (1)
97-114: Error handling for assumeRole is implicit.The
assumeRolecall (lines 104-107) can throw exceptions if the role assumption fails (invalid ARN, insufficient permissions, etc.). These exceptions will propagate up throughrefresh()and eventually to the caller. This is acceptable but means error messages won't be EKS-specific. Consider whether more specific error handling would improve debugging experience.tests/VolumeSnapshotClassTest.php (1)
36-44: Simplify YAML array handling if possible.The defensive array handling (lines 37-44) suggests
fromYamlFilemight return an array in some scenarios. This adds complexity and might mask underlying issues. If this handles multi-document YAML, consider explicitly testing that case. If it's working around CRD registration quirks, document why this is necessary or fix the root cause.src/Auth/ServiceAccountTokenProvider.php (1)
102-129: Reflection-based state manipulation is fragile but necessary.The
makeBootstrapRequestmethod uses reflection to temporarily swap the cluster's authentication state. While this is a code smell (direct manipulation of private properties), it's a pragmatic solution to the infinite recursion problem when a provider needs to use the cluster to obtain tokens.Consider adding a dedicated method on
KubernetesClusterlikecallWithToken(string $token, ...)in the future to avoid reflection, though the current approach works.src/Kinds/K8sPod.php (1)
427-435: ConsidertryFrom()for defensive handling of unknown phases.The current implementation uses
PodPhase::from(), which throws aValueErrorif the phase string doesn't match any enum case. While Kubernetes phases are well-defined (Pending,Running,Succeeded,Failed,Unknown), if a future Kubernetes version introduces a new phase or the API returns something unexpected, this will crash.Since you already have
PodPhase::UNKNOWN, usingtryFrom()with a fallback would be more resilient:🔎 Optional defensive fallback
public function getPodPhase(): PodPhase { $phase = $this->getPhase(); - return PodPhase::from($phase ?: PodPhase::UNKNOWN->value); + return PodPhase::tryFrom($phase ?? '') ?? PodPhase::UNKNOWN; }This is a nitpick — the current code is fine if you prefer to fail fast on unexpected values.
src/KubernetesCluster.php (1)
190-195: Stale docblocks:@return boolbut methods returnmixed.Lines 193 and 279 have docblocks claiming
@return bool, but bothwatchPath()andwatchLogsPath()now returnmixed(can benull, the callback result, etc.). The docblocks are misleading.🔎 Update the docblocks
For
watchPath:/** * Watch for the current resource or a resource list. - * - * @return bool */ protected function watchPath(string $path, Closure $callback, array $query = ['pretty' => 1]): mixedFor
watchLogsPath:/** * Watch for the logs for the resource. - * - * @return bool */ protected function watchLogsPath(string $path, Closure $callback, array $query = ['pretty' => 1]): mixedSince PHP 8.2 has native return types, the redundant (and now incorrect) PHPDoc return types just add noise.
Also applies to: 275-280
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (64)
.github/dependabot.yml.github/workflows/ci.ymlAGENTS.mdPATCH_SUPPORT.mdUPGRADING.mdcomposer.jsonexamples/patch_examples.phpsrc/Auth/EksTokenProvider.phpsrc/Auth/ExecCredentialProvider.phpsrc/Auth/OpenShiftOAuthProvider.phpsrc/Auth/ServiceAccountTokenProvider.phpsrc/Auth/TokenProvider.phpsrc/Contracts/TokenProviderInterface.phpsrc/Enums/AccessMode.phpsrc/Enums/ConditionStatus.phpsrc/Enums/Operation.phpsrc/Enums/PodPhase.phpsrc/Enums/Protocol.phpsrc/Enums/PullPolicy.phpsrc/Enums/RestartPolicy.phpsrc/Enums/ServiceType.phpsrc/Exceptions/AuthenticationException.phpsrc/Instances/Instance.phpsrc/Instances/ResourceMetric.phpsrc/Kinds/K8sCSIDriver.phpsrc/Kinds/K8sCSINode.phpsrc/Kinds/K8sPod.phpsrc/Kinds/K8sResource.phpsrc/Kinds/K8sScale.phpsrc/Kinds/K8sService.phpsrc/Kinds/K8sVerticalPodAutoscaler.phpsrc/Kinds/K8sVolumeAttributesClass.phpsrc/KubernetesCluster.phpsrc/Patches/JsonMergePatch.phpsrc/Patches/JsonPatch.phpsrc/Traits/Cluster/AuthenticatesCluster.phpsrc/Traits/Cluster/LoadsFromKubeConfig.phpsrc/Traits/Cluster/MakesHttpCalls.phpsrc/Traits/Cluster/MakesWebsocketCalls.phpsrc/Traits/InitializesResources.phpsrc/Traits/RunsClusterOperations.phptests/Auth/EksTokenProviderTest.phptests/Auth/ExecCredentialIntegrationTest.phptests/Auth/ExecCredentialProviderTest.phptests/Auth/OpenShiftOAuthProviderTest.phptests/Auth/ServiceAccountTokenIntegrationTest.phptests/Auth/TokenProviderTest.phptests/CSIDriverTest.phptests/CSINodeTest.phptests/CronJobTest.phptests/JobTest.phptests/Kinds/VolumeSnapshotClass.phptests/Kinds/VolumeSnapshotContent.phptests/KubeConfigTest.phptests/VolumeAttributesClassTest.phptests/VolumeSnapshotClassTest.phptests/VolumeSnapshotContentTest.phptests/cluster/exec-credential.jsontests/yaml/csidriver.yamltests/yaml/csinode.yamltests/yaml/kubeconfig-exec.yamltests/yaml/volumeattributesclass.yamltests/yaml/volumesnapshotclass.yamltests/yaml/volumesnapshotcontent.yaml
💤 Files with no reviewable changes (2)
- PATCH_SUPPORT.md
- examples/patch_examples.php
🧰 Additional context used
🧬 Code graph analysis (35)
src/Instances/ResourceMetric.php (4)
src/Kinds/K8sResource.php (1)
toArray(116-133)src/Patches/JsonMergePatch.php (1)
toArray(119-122)src/Patches/JsonPatch.php (1)
toArray(153-156)src/Instances/Instance.php (1)
toArray(23-26)
src/Patches/JsonPatch.php (2)
src/Kinds/K8sResource.php (1)
toJson(141-144)src/Patches/JsonMergePatch.php (1)
toJson(129-132)
tests/Auth/OpenShiftOAuthProviderTest.php (1)
src/Auth/OpenShiftOAuthProvider.php (3)
OpenShiftOAuthProvider(9-129)withoutSslVerification(37-42)withOAuthEndpoint(30-35)
src/Kinds/K8sVerticalPodAutoscaler.php (1)
src/Traits/Resource/HasSpec.php (1)
setSpec(13-16)
src/Patches/JsonMergePatch.php (2)
src/Kinds/K8sResource.php (1)
toJson(141-144)src/Patches/JsonPatch.php (1)
toJson(163-166)
src/Kinds/K8sResource.php (2)
src/Patches/JsonMergePatch.php (1)
toJson(129-132)src/Patches/JsonPatch.php (1)
toJson(163-166)
src/Auth/TokenProvider.php (2)
src/Contracts/TokenProviderInterface.php (4)
getToken(12-12)isExpired(17-17)refresh(22-22)getExpiresAt(27-27)tests/Auth/TokenProviderTest.php (2)
refresh(14-18)refresh(25-30)
src/Contracts/TokenProviderInterface.php (6)
src/Auth/TokenProvider.php (4)
getToken(19-26)isExpired(28-37)refresh(54-54)getExpiresAt(39-42)src/Auth/EksTokenProvider.php (1)
refresh(45-87)src/Auth/ExecCredentialProvider.php (1)
refresh(49-84)src/Auth/OpenShiftOAuthProvider.php (1)
refresh(44-94)src/Auth/ServiceAccountTokenProvider.php (1)
refresh(49-96)tests/Auth/TokenProviderTest.php (2)
refresh(14-18)refresh(25-30)
src/Traits/Cluster/LoadsFromKubeConfig.php (2)
src/Auth/ExecCredentialProvider.php (2)
ExecCredentialProvider(8-106)setClusterInfo(42-47)src/Traits/Cluster/AuthenticatesCluster.php (1)
withTokenProvider(212-218)
tests/CronJobTest.php (2)
src/Traits/InitializesResources.php (1)
pod(192-195)src/Kinds/K8sPod.php (1)
getRestartPolicy(236-239)
tests/VolumeSnapshotContentTest.php (3)
src/ResourcesList.php (1)
ResourcesList(7-10)tests/Kinds/VolumeSnapshotContent.php (18)
VolumeSnapshotContent(11-228)setDeletionPolicy(43-46)setDriver(63-66)setSnapshotHandle(126-129)setSourceVolumeMode(166-169)setVolumeSnapshotClassName(106-109)setVolumeSnapshotRef(83-89)getDeletionPolicy(53-56)getDriver(73-76)getSnapshotHandle(136-139)getSourceVolumeMode(176-179)getVolumeSnapshotClassName(116-119)getVolumeSnapshotRef(96-99)isReady(184-187)getRestoreSize(194-197)getSnapshotHandleFromStatus(204-207)getCreationTime(214-217)getError(224-227)src/Traits/Resource/HasVersion.php (1)
getApiVersion(39-42)
src/Auth/ExecCredentialProvider.php (4)
src/Exceptions/AuthenticationException.php (1)
AuthenticationException(7-10)src/Auth/TokenProvider.php (2)
TokenProvider(8-55)refresh(54-54)src/Contracts/TokenProviderInterface.php (1)
refresh(22-22)src/Instances/Container.php (1)
setEnv(223-235)
src/Auth/EksTokenProvider.php (6)
src/Exceptions/AuthenticationException.php (1)
AuthenticationException(7-10)src/Auth/TokenProvider.php (2)
TokenProvider(8-55)refresh(54-54)src/Auth/ExecCredentialProvider.php (2)
__construct(24-40)refresh(49-84)src/Auth/OpenShiftOAuthProvider.php (2)
__construct(23-28)refresh(44-94)src/Auth/ServiceAccountTokenProvider.php (2)
__construct(22-33)refresh(49-96)src/Contracts/TokenProviderInterface.php (1)
refresh(22-22)
src/Enums/ServiceType.php (1)
src/Kinds/K8sService.php (1)
isExternallyAccessible(58-61)
src/Traits/Cluster/AuthenticatesCluster.php (4)
src/Contracts/TokenProviderInterface.php (1)
getToken(12-12)src/Auth/EksTokenProvider.php (1)
EksTokenProvider(7-118)src/Auth/OpenShiftOAuthProvider.php (2)
OpenShiftOAuthProvider(9-129)withoutSslVerification(37-42)src/Auth/ServiceAccountTokenProvider.php (3)
ServiceAccountTokenProvider(8-130)withExpirationSeconds(35-40)withAudiences(42-47)
tests/Auth/EksTokenProviderTest.php (1)
src/Auth/EksTokenProvider.php (5)
EksTokenProvider(7-118)isAvailable(25-29)refresh(45-87)withProfile(31-36)withAssumeRole(38-43)
src/Instances/Instance.php (4)
src/Instances/ResourceMetric.php (1)
toArray(142-147)src/Kinds/K8sResource.php (1)
toArray(116-133)src/Patches/JsonMergePatch.php (1)
toArray(119-122)src/Patches/JsonPatch.php (1)
toArray(153-156)
tests/VolumeSnapshotClassTest.php (4)
tests/Kinds/VolumeSnapshotClass.php (7)
VolumeSnapshotClass(8-88)setDriver(36-39)setDeletionPolicy(56-59)setParameters(76-79)getDriver(46-49)getDeletionPolicy(66-69)getParameters(84-87)src/Traits/Resource/HasLabels.php (2)
setLabels(12-15)getLabels(20-23)src/Traits/Resource/HasVersion.php (1)
getApiVersion(39-42)src/Kinds/K8sResource.php (1)
getByName(105-108)
src/Kinds/K8sCSIDriver.php (1)
src/Traits/Resource/HasSpec.php (2)
setSpec(13-16)getSpec(34-37)
tests/Kinds/VolumeSnapshotClass.php (2)
src/Traits/Resource/HasAttributes.php (2)
setAttribute(44-49)getAttribute(85-88)src/Kinds/K8sVolumeAttributesClass.php (2)
setParameters(56-59)getParameters(64-67)
src/Kinds/K8sPod.php (3)
src/Traits/Resource/HasSpec.php (2)
setSpec(13-16)getSpec(34-37)src/Traits/Resource/HasStatusPhase.php (1)
getPhase(14-17)src/Enums/PodPhase.php (2)
isSuccessful(45-48)isTerminal(23-29)
tests/Auth/ExecCredentialIntegrationTest.php (3)
tests/TestCase.php (1)
TestCase(12-199)src/Traits/Cluster/LoadsFromKubeConfig.php (2)
fromKubeConfigArray(104-109)fromKubeConfigYamlFile(94-97)src/Traits/Cluster/AuthenticatesCluster.php (1)
getAuthToken(224-231)
src/Kinds/K8sVolumeAttributesClass.php (2)
src/Traits/Resource/HasAttributes.php (2)
setAttribute(44-49)getAttribute(85-88)tests/Kinds/VolumeSnapshotClass.php (2)
setParameters(76-79)getParameters(84-87)
src/Traits/Cluster/MakesHttpCalls.php (1)
src/Traits/Cluster/AuthenticatesCluster.php (1)
getAuthToken(224-231)
src/Traits/InitializesResources.php (3)
src/Kinds/K8sCSIDriver.php (1)
K8sCSIDriver(9-179)src/Kinds/K8sCSINode.php (1)
K8sCSINode(9-101)src/Kinds/K8sVolumeAttributesClass.php (1)
K8sVolumeAttributesClass(8-68)
tests/Auth/TokenProviderTest.php (2)
src/Auth/TokenProvider.php (6)
TokenProvider(8-55)refresh(54-54)isExpired(28-37)getToken(19-26)setRefreshBuffer(47-52)getExpiresAt(39-42)src/Contracts/TokenProviderInterface.php (4)
refresh(22-22)isExpired(17-17)getToken(12-12)getExpiresAt(27-27)
src/Auth/OpenShiftOAuthProvider.php (3)
src/Exceptions/AuthenticationException.php (1)
AuthenticationException(7-10)src/Auth/TokenProvider.php (2)
TokenProvider(8-55)refresh(54-54)src/Contracts/TokenProviderInterface.php (1)
refresh(22-22)
tests/CSINodeTest.php (5)
src/Kinds/K8sCSINode.php (7)
K8sCSINode(9-101)getDrivers(37-40)hasDriver(63-66)getDriverByName(47-58)getNodeIdForDriver(73-78)getTopologyKeysForDriver(83-88)getAllocatableForDriver(95-100)src/ResourcesList.php (1)
ResourcesList(7-10)src/Traits/InitializesResources.php (1)
csiNode(170-173)src/Traits/Resource/HasSpec.php (1)
setSpec(13-16)src/K8s.php (1)
fromYamlFile(57-66)
tests/Kinds/VolumeSnapshotContent.php (2)
src/Traits/Resource/HasSpec.php (2)
setSpec(13-16)getSpec(34-37)src/Traits/Resource/HasStatus.php (1)
getStatus(12-15)
src/Kinds/K8sService.php (6)
src/Kinds/K8sPod.php (1)
getClusterDns(48-53)src/Contracts/Dnsable.php (1)
getClusterDns(12-12)src/Traits/Resource/HasName.php (1)
getName(36-39)src/Traits/Resource/HasNamespace.php (1)
getNamespace(73-76)src/Traits/Resource/HasSpec.php (3)
setSpec(13-16)getSpec(34-37)addToSpec(24-27)src/Enums/ServiceType.php (1)
isExternallyAccessible(20-26)
src/Traits/Cluster/MakesWebsocketCalls.php (1)
src/Traits/Cluster/AuthenticatesCluster.php (1)
getAuthToken(224-231)
tests/JobTest.php (2)
src/Traits/InitializesResources.php (1)
pod(192-195)src/Kinds/K8sPod.php (1)
getRestartPolicy(236-239)
src/Kinds/K8sCSINode.php (1)
src/Traits/Resource/HasSpec.php (1)
getSpec(34-37)
tests/VolumeAttributesClassTest.php (7)
src/Kinds/K8sVolumeAttributesClass.php (5)
K8sVolumeAttributesClass(8-68)setDriverName(36-39)setParameters(56-59)getDriverName(46-49)getParameters(64-67)src/Traits/InitializesResources.php (1)
volumeAttributesClass(181-184)src/Traits/Resource/HasLabels.php (2)
setLabels(12-15)getLabels(20-23)src/Traits/Resource/HasVersion.php (1)
getApiVersion(39-42)src/Traits/Resource/HasAttributes.php (1)
isSynced(116-119)src/Traits/RunsClusterOperations.php (3)
createOrUpdate(131-140)delete(255-282)watch(407-423)src/Kinds/K8sResource.php (1)
watchByName(173-176)
src/KubernetesCluster.php (2)
src/Enums/Operation.php (1)
httpMethod(29-38)src/K8s.php (1)
__call(122-129)
🪛 LanguageTool
AGENTS.md
[style] ~54-~54: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...les) to avoid cluster requirements. - Run specific test file: `vendor/bin/phpun...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~56-~56: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...-filter test_priority_class_build. - **Run with CI environment:** CI=true vendor/...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~106-~106: Ensure spelling is correct
Context: ...luster. Common Tasks - Add a new builtin kind: 1. Create `src/Kinds/K8sYourK...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~202-~202: Consider a different adjective to strengthen your wording.
Context: ...pi/ If anything is unclear or you need deeper context for a change, open a draft PR w...
(DEEP_PROFOUND)
🪛 markdownlint-cli2 (0.18.1)
AGENTS.md
1-1: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
5-5: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
15-15: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
42-42: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
49-49: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
80-80: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
105-105: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
148-148: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
155-155: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
162-162: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
172-172: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
183-183: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
192-192: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
197-197: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
198-198: Bare URL used
(MD034, no-bare-urls)
199-199: Bare URL used
(MD034, no-bare-urls)
200-200: Bare URL used
(MD034, no-bare-urls)
🪛 PHPMD (2.15.0)
tests/CSIDriverTest.php
149-149: Avoid unused parameters such as '$type'. (undefined)
(UnusedFormalParameter)
160-160: Avoid unused parameters such as '$type'. (undefined)
(UnusedFormalParameter)
tests/VolumeAttributesClassTest.php
154-154: Avoid unused parameters such as '$type'. (undefined)
(UnusedFormalParameter)
165-165: Avoid unused parameters such as '$type'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (72)
src/Kinds/K8sVerticalPodAutoscaler.php (2)
52-55: LGTM - Proper return type for fluent interface.The
: staticreturn type correctly declares that this method returns the instance for chaining. This aligns with the actual behavior (returning$thisviasetSpec) and improves type safety.
60-63: LGTM - Proper return type for fluent interface.The
: staticreturn type correctly declares that this method returns the instance for chaining. This aligns with the actual behavior (returning$thisviasetSpec) and improves type safety.src/Instances/Instance.php (1)
20-26: LGTM - Proper interface compliance.The explicit
: arrayreturn type brings this implementation into proper compliance with theArrayableinterface contract. The return type is correct since$this->attributesis initialized as an array.src/Instances/ResourceMetric.php (1)
139-147: LGTM - Consistent with parent class.The explicit
: arrayreturn type is correct and consistent with the parentInstance::toArray()signature. The implementation usingarray_merge()guarantees an array return, so the type hint is accurate.src/Enums/PullPolicy.php (3)
10-14: LGTM - Enum definition is correct.The three cases map directly to Kubernetes ImagePullPolicy values and the string backing type enables seamless API serialization.
19-22: LGTM - Logic is correct.The method correctly identifies the ALWAYS policy. The straightforward comparison is appropriate here.
27-33: LGTM - Semantics are correct.The logic correctly identifies which policies permit using cached images. IF_NOT_PRESENT pulls only when absent locally, NEVER requires the cached image, and ALWAYS pulls fresh. The match expression is the right tool for this multi-case scenario.
src/Enums/AccessMode.php (4)
10-15: Enum declaration is correct.All four persistent volume access modes match the official Kubernetes API specification. The string values are properly formatted for K8s resource definitions.
17-26: Logic is correct.The method accurately identifies which access modes permit write operations. Match expression is exhaustive.
28-37: Logic is correct.The method correctly identifies access modes that allow multiple nodes (or pods in RWOP's case) to mount the volume. Match expression is exhaustive.
39-45: Logic is correct.Direct comparison is appropriate here since only
READ_WRITE_ONCE_PODis pod-scoped. Using===instead of a match expression is more concise for single-case checks.src/Enums/ConditionStatus.php (2)
10-14: LGTM - Enum structure is correct.The string-backed enum correctly represents Kubernetes condition statuses with proper capitalization ('True', 'False', 'Unknown') matching the API spec.
19-38: Helper methods are fine but entirely optional.Brutal honesty: These methods are syntactic sugar. PHP enums can be compared directly (
$status === ConditionStatus::TRUEis equally readable), soisTrue()andisFalse()don't add much. TheisKnown()method provides slightly more value by encapsulating the negation.That said, they're not wrong—they provide a fluent API that some developers prefer, and they're self-documenting. The implementation is correct.
src/Enums/Protocol.php (1)
10-14: LGTM – Protocol cases are correct.TCP, UDP, and SCTP are the three protocols Kubernetes supports for ports and services. String backing is appropriate for direct manifest serialization.
src/Enums/ServiceType.php (1)
10-35: Solid enum implementation.The string-backed values correctly match Kubernetes API expectations. The
isExternallyAccessible()logic is accurate—NodePort and LoadBalancer are indeed the types that expose traffic beyond the cluster. Usingmatchhere is cleaner than a switch. No issues.src/Kinds/K8sService.php (2)
39-61: Clean enum integration.Storing
$type->valueand usingServiceType::from()with a sensible default is the right approach. ClusterIP is indeed Kubernetes' default, so that's accurate. The delegation togetType()->isExternallyAccessible()keeps logic where it belongs.
63-97: Port methods look fine.Fluent interface is consistent, defaults are sensible. The
addPortsloop isn't the most efficient approach if you're adding hundreds of ports, but that's not a realistic scenario for Kubernetes services. No issues here..github/dependabot.yml (1)
3-16: LGTM - Structural formatting fix.The YAML flattening removes unnecessary nesting without changing Dependabot's behavior. Configuration is standard and correct.
.github/workflows/ci.yml (3)
24-26: Quote style change is purely cosmetic.Single-quoting version strings has no functional impact in YAML. The change is stylistic.
106-107: GITHUB_TOKEN addition is sensible.Adding the token to minikube setup will help avoid GitHub API rate limits when pulling Kubernetes images or metadata. Good call.
93-93: actions/cache@v5 is stable and released. v5.0.0 is an official release with full documentation. The upgrade from v4 to v5 carries no stability risk. Verify your CI runner meets the minimum version requirement (2.327.1+) due to the cache service migration.composer.json (4)
28-29: Inconsistency: Are these dependencies removed or not?The AI summary claims
ext-jsonandcomposer/semverwere "removed" and the "final manifest omits these entries." But the annotated code shows them right here with change markers, indicating they're in the final state. Which is it?Looking at the actual lines, these dependencies ARE present. The summary is flat-out wrong.
37-38: AWS SDK suggestion aligns with EKS token provider.Adding
aws/aws-sdk-phpto the suggest section makes sense given the PR introduces native EKS token generation. The description clearly states the use case.
34-34: Version 7.3.4 exists and is stable—no action needed.The constraint
^7.3.4is legitimate. Packagist confirms symfony/process v7.3.4 is a stable release.
56-58: Both versions are confirmed released and available on Packagist. No action needed.
orchestra/testbench v10.8.0released 2025-11-24vimeo/psalm v6.14.2is availableThe dependency bump is safe.
Likely an incorrect or invalid review comment.
src/Exceptions/AuthenticationException.php (1)
1-10: LGTM - Standard marker exception pattern.Empty exception class is the correct approach here. The type itself provides semantic value for catch blocks and allows authentication-specific error handling without requiring additional methods.
tests/cluster/exec-credential.json (1)
1-8: LGTM - Valid test fixture.The ExecCredential structure is correct per the client.authentication.k8s.io/v1 spec. Far-future expiration (2099) prevents test flakiness from token expiry.
tests/yaml/volumesnapshotcontent.yaml (1)
1-14: LGTM - Valid VolumeSnapshotContent manifest.The YAML structure correctly defines a VolumeSnapshotContent with all required fields for the snapshot.storage.k8s.io/v1 API. Appropriate for test coverage.
AGENTS.md (1)
2-203: Documentation content is comprehensive and well-structured.The AGENTS.md file provides excellent guidance for contributors covering setup, testing, architecture patterns, and development workflows. Once the markdown linting issues are resolved, this will be a solid developer resource.
tests/yaml/csinode.yaml (1)
1-13: LGTM - Valid CSINode manifest.The YAML correctly defines a CSINode resource with driver configuration including nodeID, topology keys, and allocatable count. Appropriate for testing CSI resource handling.
src/Traits/Cluster/MakesHttpCalls.php (1)
84-87: LGTM - Correct token provider integration.The switch from direct
$this->tokento$this->getAuthToken()properly integrates the new token provider architecture. The method correctly prioritizes dynamic tokens from providers (EKS, exec, OpenShift, ServiceAccount) while maintaining backward compatibility with static tokens.tests/CronJobTest.php (1)
7-7: LGTM - Correct enum migration.The test correctly migrates from string comparison (
'Never') to enum comparison (RestartPolicy::NEVER). This aligns with the type-safe modernization effort and matches the updatedK8sPod::getRestartPolicy()signature that now returns theRestartPolicyenum.Also applies to: 44-44, 66-66, 124-124
tests/yaml/kubeconfig-exec.yaml (1)
1-20: LGTM - Clever test fixture for exec authentication.The kubeconfig correctly defines exec-based authentication with a clever testing approach: using
echoto output the ExecCredential JSON directly. This avoids needing a real credential plugin binary while still exercising the full exec provider code path.tests/JobTest.php (1)
5-5: LGTM on enum migration.The update to use
RestartPolicy::NEVERenum instead of the string literal is correct and improves type safety. The underlying pod configuration still uses the string'Never'(lines 16, 39), which is appropriate since YAML/array attributes use strings. ThegetRestartPolicy()method correctly converts the string to the enum.Also applies to: 31-31, 49-49
src/Traits/Cluster/LoadsFromKubeConfig.php (1)
221-237: Correct implementation of exec credential provider integration.The logic properly detects the
user.execconfiguration, creates anExecCredentialProvider, and conditionally injects cluster information whenprovideClusterInfois set. The cluster info structure (server, certificate-authority-data, insecure-skip-tls-verify) matches what the provider expects, and the null coalescing on line 231 correctly handles optional certificate data.src/Patches/JsonPatch.php (1)
163-166: Correct return type for JSON encoding.The updated return type
string|falseaccurately reflects thatjson_encode()can fail and returnfalse. This change improves type safety and is consistent with the same updates inJsonMergePatchandK8sResource.src/Kinds/K8sResource.php (1)
141-144: Correct return type for JSON encoding.The
string|falsereturn type accurately reflectsjson_encode()'s behavior and improves type safety, consistent with the updates inJsonPatchandJsonMergePatch.src/Kinds/K8sScale.php (1)
6-6: Clean enum migration for scale operations.The replacement of
KubernetesCluster::REPLACE_OPwithOperation::REPLACEis correct and consistent with the PR's modernization goals. Bothcreate()andupdate()appropriately use theREPLACEoperation for scale subresources, as documented in the comments explaining that scale doesn't support POST operations.Also applies to: 101-101, 124-124
src/Enums/RestartPolicy.php (1)
10-30: Clean enum implementation.The enum correctly maps to Kubernetes restart policy values, and the helper methods provide useful semantic queries.
allowsRestarts()correctly excludes onlyNEVER, andonlyOnFailure()is a straightforward identity check.src/Traits/RunsClusterOperations.php (2)
11-11: Import addition is correct.The Operation enum import is properly added to support the migration from constants.
149-159: Enum migration done consistently throughout.The switch from
KubernetesCluster::GET_OPtoOperation::GET(and similar for all other operations) is applied uniformly across all methods. This is a clean, mechanical refactor with no logic changes.UPGRADING.md (1)
1-103: Solid upgrade documentation.The before/after examples are clear, the migration paths are well-explained, and the backward compatibility note (strings still work for
runOperation) is helpful. The troubleshooting section addresses the obvious friction points.src/Enums/Operation.php (1)
10-49: Solid enum design.HTTP method mappings are correct per Kubernetes API conventions:
APPLYusesPATCH(server-side apply)LOGusesGET(fetches log content)- WebSocket detection covers the right operations
The exhaustive
matchexpressions mean adding a new operation will fail at compile-time if you forget to update the methods. This is exactly what you want.tests/KubeConfigTest.php (4)
102-104: SSL array normalization is fine.The expected options now explicitly include an empty
'ssl' => []rather than omitting the key. This reflects the implementation always returning the ssl key. No behavioral change.Also applies to: 121-123
277-303: Inline JSON in test args is fragile but acceptable.The embedded JSON in
args(line 294) is a validExecCredentialresponse. If the parser becomes stricter or the format changes, you'll need to update this string. Consider extracting to a constant or fixture file if this pattern repeats.
305-323: Good coverage of provider priority.This test explicitly verifies that
withTokenProvider()takes precedence overwithToken(). That's critical behavior for users who set both accidentally.
260-275: The test fixturetests/yaml/kubeconfig-exec.yamlexists in the repository. No action required.src/Enums/PodPhase.php (1)
10-48: Enum correctly models Kubernetes pod phases.The phase values match the Kubernetes API, and the helper methods provide useful semantic queries. One observation: based on the relevant snippets,
K8sPod::isSuccessful()uses direct comparison (=== PodPhase::SUCCEEDED) whileK8sPod::isTerminal()delegates to the enum's method. Consider havingK8sPod::isSuccessful()also delegate to$this->getPodPhase()->isSuccessful()for consistency—but this is purely a style nit.src/Contracts/TokenProviderInterface.php (1)
7-28: LGTM! Clean interface design.The interface is well-structured with clear method signatures and documentation. The contract appropriately separates concerns for token retrieval, expiration checking, and refresh operations.
src/Traits/Cluster/MakesWebsocketCalls.php (1)
51-56: LGTM! Consistent token provider integration.The WebSocket authentication flow now correctly uses
getAuthToken(), maintaining consistency with HTTP calls and supporting the new token provider mechanism. The fallback to Basic auth ensures backward compatibility.Also applies to: 107-112
src/Traits/InitializesResources.php (1)
153-184: LGTM! Consistent factory method pattern.The new CSI resource factory methods follow the established pattern perfectly. Documentation and signatures are consistent with existing resource factories.
tests/Kinds/VolumeSnapshotClass.php (1)
1-88: LGTM! Standard resource implementation.This test resource class follows the established pattern for K8s resources. The getters/setters are straightforward and correctly use the attribute helper methods.
tests/Auth/EksTokenProviderTest.php (1)
1-91: LGTM! Well-structured conditional tests.The test suite appropriately handles the optional AWS SDK dependency with conditional test execution. Using reflection to verify internal configuration is a reasonable approach for testing fluent configuration methods without requiring live AWS credentials.
tests/VolumeSnapshotContentTest.php (1)
36-60: LGTM!The test correctly handles the case where
fromYamlFile()may return an array when CRD registration is involved. The defensive iteration (lines 43-50) to extract the specific instance type is appropriate for CRD scenarios where multiple resources might be defined in a single YAML file.tests/Auth/ServiceAccountTokenIntegrationTest.php (1)
32-66: LGTM!The integration test properly validates ServiceAccount token lifecycle: creates a test SA, obtains a token with 10-minute expiration, verifies JWT format and expiration window (500-700s with reasonable buffer), and ensures cleanup via finally block. The test structure is solid.
tests/Auth/ExecCredentialProviderTest.php (2)
11-119: LGTM!Comprehensive test coverage for ExecCredentialProvider including:
- Basic token extraction from JSON
- Expiration timestamp handling
- Multiple API versions (v1 and v1beta1)
- Environment variable injection
- Error cases (missing token, invalid JSON, failed command)
The error handling tests properly verify exception types and messages.
121-151: LGTM!The cluster info test correctly validates that
KUBERNETES_EXEC_INFOis set whenprovideClusterInfois enabled. Using reflection to inspect the privateclusterInfoproperty (lines 145-148) is appropriate for unit testing internal state. The try-catch acknowledges thatprintenvoutput isn't valid JSON, which is expected for this specific test.tests/Auth/TokenProviderTest.php (2)
8-31: LGTM!The test helper classes (
MockTokenProviderandCountingTokenProvider) are well-designed for testing the abstractTokenProviderbehavior.CountingTokenProviderelegantly tracks refresh calls to verify lazy-refresh logic, whileMockTokenProviderenables controlled expiration testing.
35-98: LGTM!Comprehensive test coverage of the TokenProvider expiration logic:
- Non-expiring tokens (null expiresAt)
- Future expiration (far future)
- Past expiration
- Buffer-zone expiration (within 60s default buffer)
- Lazy refresh triggering
- Custom refresh buffer configuration
The tests correctly validate that tokens within the refresh buffer (e.g., +30s on line 64) are considered expired, ensuring proactive refresh.
tests/CSINodeTest.php (1)
1-107: LGTM!The test structure is solid and follows established patterns. The conditional handling for environments without CSI drivers (lines 91-104) is appropriate, and driver accessor tests cover both positive and negative cases.
tests/Auth/ExecCredentialIntegrationTest.php (1)
14-16: Verify CI cluster availability assumptions.The skip logic assumes clusters are always available in CI environments. If CI can run without a cluster, these tests will fail hard instead of skipping. Confirm this matches your CI setup expectations.
src/Kinds/K8sPod.php (5)
11-12: Clean enum imports for type-safe pod lifecycle handling.These imports enable the enum-based refactoring throughout the class.
212-215: LGTM on the restart policy helper methods.Both methods correctly use the enum's backing value. The fluent
staticreturn type is consistent with the rest of the class.Also applies to: 220-223
226-239: Solid enum-based restart policy implementation.The setter accepts the enum directly, and the getter correctly defaults to
RestartPolicy::ALWAYSwhich matches Kubernetes' default behavior. One thing to be aware of:RestartPolicy::from()will throwValueErrorif the spec contains an unrecognized value. Given that restart policies are a closed set defined by the Kubernetes API (Always,OnFailure,Never), this is the right call — blowing up on an invalid value is better than silently misbehaving.
437-451: Clean enum-based comparisons for pod state checks.Using enum comparisons instead of string comparisons is objectively better — no more typo-induced bugs like
'Runing'vs'Running'. The logic is correct.
453-459: Good addition ofisTerminal()method.Delegating to
PodPhase->isTerminal()keeps the logic in one place. This will be useful for job/batch workload monitoring where you need to know if a pod has reached a final state.src/KubernetesCluster.php (6)
140-145: Explicit nullable type hints are the right move.The
?stringtypes properly document that these properties can be null, which was probably always the case but now it's explicit.
171-188: Clean enum-based operation dispatch with backward compatibility.The
Operation|stringunion type with internal conversion is a smart approach — existing code passing strings won't break immediately, but you're steering toward the enum. The match expression is far more readable than the previous switch/if chain would have been.One note:
Operation::from($operation)will throwValueErrorif someone passes an invalid string. That's the right behavior — fail fast on garbage input.
349-369: Enum-based HTTP method resolution inexecPath().Using
Operation::EXEC->httpMethod()instead of hardcoding 'POST' means the Operation enum is now the single source of truth for method mappings. Solid.
377-398: Same pattern forattachPath()— consistent and correct.
406-449: Patch operation methods (applyPath,jsonPatchPath,jsonMergePatchPath) all follow the same enum pattern.The Content-Type headers are still correctly set for each patch type, and the HTTP method comes from the enum. No issues here.
454-497: Explicit type hints on__callmagic method.
string $method, array $parametersandmixedreturn — this is just telling the truth about what's already happening. The method body is unchanged and correctly proxies to K8s class methods.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
AGENTS.md (3)
1-1: Use proper markdown headings instead of bold text for section titles.This was flagged in the previous review. All 14 section headings should be converted from bold (
**Section**) to markdown H2 (## Section) for proper document structure, navigation, and accessibility.🔎 Proposed fix for section headings
-**Purpose** +## Purpose -**Quick Repo Map** +## Quick Repo Map -**Code Style Requirements** +## Code Style Requirements -**Stack & Requirements** +## Stack & Requirements -**Local Dev Setup** +## Local Dev Setup -**Running Full Integration Tests Locally** +## Running Full Integration Tests Locally -**Key Concepts** +## Key Concepts -**Common Tasks** +## Common Tasks -**Style & Quality** +## Style & Quality -**PR Checklist** +## PR Checklist -**Gotchas** +## Gotchas -**Current Resource Types (33+)** +## Current Resource Types (33+) -**Kubernetes API Versions Reference** +## Kubernetes API Versions Reference -**Releasing & CI (Reference)** +## Releasing & CI (Reference) -**Useful References** +## Useful ReferencesAlso applies to: 5-5, 15-15, 42-42, 49-49, 80-80, 105-105, 148-148, 155-155, 162-162, 172-172, 183-183, 192-192, 197-197
198-200: Format bare URLs as markdown links.This was flagged in the previous review. All three URLs should be wrapped in markdown link syntax for proper rendering and clickability.
🔎 Proposed fix
-- **Documentation:** https://php-k8s.renoki.org -- **Upstream Repository:** https://github.com/renoki-co/php-k8s -- **Kubernetes API Reference:** https://kubernetes.io/docs/reference/kubernetes-api/ +- **Documentation:** [https://php-k8s.renoki.org](https://php-k8s.renoki.org) +- **Upstream Repository:** [https://github.com/renoki-co/php-k8s](https://github.com/renoki-co/php-k8s) +- **Kubernetes API Reference:** [https://kubernetes.io/docs/reference/kubernetes-api/](https://kubernetes.io/docs/reference/kubernetes-api/)
193-193: Update CI matrix documentation to reflect tested PHP versions.Line 193 claims the CI matrix tests PHP 8.3/8.4, but the PR objectives state that CI was updated to test PHP 8.2, 8.3, 8.4, and 8.5. Update the documentation to accurately reflect the actual tested versions.
🔎 Proposed fix
-- CI matrix runs PHP 8.3/8.4 across Kubernetes v1.32.9, v1.33.5, v1.34.1 and Laravel 11/12 with both `prefer-lowest` and `prefer-stable`. +- CI matrix runs PHP 8.2, 8.3, 8.4, and 8.5 across Kubernetes v1.32.9, v1.33.5, v1.34.1 and Laravel 11/12 with both `prefer-lowest` and `prefer-stable`.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
AGENTS.md
🧰 Additional context used
🪛 LanguageTool
AGENTS.md
[style] ~54-~54: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...les) to avoid cluster requirements. - Run specific test file: `vendor/bin/phpun...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~56-~56: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...-filter test_priority_class_build. - **Run with CI environment:** CI=true vendor/...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~202-~202: Consider a different adjective to strengthen your wording.
Context: ...pi/ If anything is unclear or you need deeper context for a change, open a draft PR w...
(DEEP_PROFOUND)
🪛 markdownlint-cli2 (0.18.1)
AGENTS.md
1-1: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
5-5: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
15-15: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
42-42: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
49-49: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
80-80: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
105-105: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
148-148: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
155-155: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
162-162: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
172-172: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
183-183: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
192-192: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
197-197: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
198-198: Bare URL used
(MD034, no-bare-urls)
199-199: Bare URL used
(MD034, no-bare-urls)
200-200: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/KubernetesCluster.php (2)
278-280: Update the docblock to match the signature.Line 278 says
@return bool, but the method signature returnsmixed(line 280). Same issue aswatchPath.🔎 Proposed fix
/** * Watch for the logs for the resource. * - * @return bool + * @return mixed */ protected function watchLogsPath(string $path, Closure $callback, array $query = ['pretty' => 1]): mixed
193-195: Update the docblock to match the signature.Line 193 says
@return bool, but the method signature returnsmixed(line 195). The signature is correct—the method can returnnull, the callback's return value, or other types—but the docblock is stale.🔎 Proposed fix
/** * Watch for the current resource or a resource list. * - * @return bool + * @return mixed */ protected function watchPath(string $path, Closure $callback, array $query = ['pretty' => 1]): mixed
🧹 Nitpick comments (1)
src/KubernetesCluster.php (1)
171-187: Enum-based dispatch is solid, but the type system allows an invalid state.The
tryFromwithGETfallback (line 175) correctly addresses the previous concern aboutValueError. The match expression cleanly routes operations.However, the default case (line 186) passes
$payload(typed asstring|null|Closure) tomakeRequest, which expectsstring. If an unknown operation enum case were added without updating the match expression, and a caller passed aClosurepayload, this would cause a type error.In practice, this is unlikely since all
Closure-using operations (WATCH, WATCH_LOGS, ATTACH) are explicit cases. But the type signature permits this invalid state.🔎 Consider tightening the payload type constraint
Option 1: Add a runtime check before the default case:
return match ($operation) { Operation::WATCH => $this->watchPath($path, $payload, $query), Operation::WATCH_LOGS => $this->watchLogsPath($path, $payload, $query), Operation::EXEC => $this->execPath($path, $query), Operation::ATTACH => $this->attachPath($path, $payload, $query), Operation::APPLY => $this->applyPath($path, $payload, $query), Operation::JSON_PATCH => $this->jsonPatchPath($path, $payload, $query), Operation::JSON_MERGE_PATCH => $this->jsonMergePatchPath($path, $payload, $query), - default => $this->makeRequest($operation->httpMethod(), $path, $payload, $query), + default => $this->makeRequest($operation->httpMethod(), $path, is_string($payload) || is_null($payload) ? $payload : '', $query), };Option 2: Split the signature into separate methods for different payload types, though that's likely overkill for this scenario.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/KubernetesCluster.phpsrc/Traits/Cluster/AuthenticatesCluster.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Traits/Cluster/AuthenticatesCluster.php (5)
src/Contracts/TokenProviderInterface.php (1)
getToken(12-12)src/Auth/TokenProvider.php (1)
getToken(19-26)src/Auth/EksTokenProvider.php (1)
EksTokenProvider(7-118)src/Auth/OpenShiftOAuthProvider.php (2)
OpenShiftOAuthProvider(9-129)withoutSslVerification(37-42)src/Auth/ServiceAccountTokenProvider.php (1)
ServiceAccountTokenProvider(8-130)
src/KubernetesCluster.php (3)
src/Traits/Cluster/MakesHttpCalls.php (1)
makeRequest(155-189)src/Enums/Operation.php (1)
httpMethod(29-38)src/K8s.php (1)
__call(122-129)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (12)
src/Traits/Cluster/AuthenticatesCluster.php (6)
19-24: LGTM!Clean property declaration for the token provider with proper typing and documentation.
207-218: LGTM!Setting the token provider and clearing the static token ensures the provider takes precedence. The explicit comment on line 215 clarifies the intent.
220-231: LGTM!The prioritization logic is correct: dynamic token provider takes precedence with a fallback to the static token for backward compatibility.
233-243: LGTM!EKS authentication helper correctly instantiates the provider and registers it via the common
withTokenProviderflow.
245-263: LGTM!OpenShift authentication helper correctly propagates the cluster's SSL verification setting (lines 258-260) to the OAuth provider, ensuring consistent behavior.
265-289: LGTM!ServiceAccount token helper correctly passes the cluster instance to the provider (line 277) and conditionally configures audiences (lines 284-286).
src/KubernetesCluster.php (6)
140-145: LGTM!Explicit nullable typing with default values improves clarity and aligns with PHP 8.2+ best practices.
158-163: LGTM!Return type of
staticproperly supports fluent chaining in subclass scenarios.
349-369: LGTM!Using
Operation::EXEC->httpMethod()(line 354) cleanly replaces the previous static operation mapping. Themixedreturn type is appropriate for handling both HTTP responses and WebSocket upgrades.
377-398: LGTM!Consistent with
execPath, usingOperation::ATTACH->httpMethod()(line 383) and returningmixedfor dual HTTP/WebSocket handling.
406-449: LGTM!The three patch methods (
applyPath,jsonPatchPath,jsonMergePatchPath) consistently use enum-based HTTP method retrieval (lines 414, 431, 448) and returnmixedfor flexibility. Clean refactor.
454-497: LGTM!Adding explicit parameter and return types to the
__callmagic method improves IDE support and type safety.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
AGENTS.md (1)
193-193: Fix CI matrix PHP version documentation — missing PHP 8.2.Line 193 states the CI matrix runs "PHP 8.3/8.4/8.5" but the PR description and line 43 both confirm PHP 8.2 is included. This inconsistency will confuse developers about what versions are actually tested.
Update line 193 to include PHP 8.2.
🔎 Proposed fix
-CI matrix runs PHP 8.3/8.4/8.5 across Kubernetes v1.32.9, v1.33.5, v1.34.1 and Laravel 11/12 with both `prefer-lowest` and `prefer-stable`. +CI matrix runs PHP 8.2/8.3/8.4/8.5 across Kubernetes v1.32.9, v1.33.5, v1.34.1 and Laravel 11/12 with both `prefer-lowest` and `prefer-stable`.
🧹 Nitpick comments (2)
AGENTS.md (1)
54-56: Refactor repetitive sentence starters to improve readability.Lines 54–56 all begin with "Run", which breaks the flow. Consider varying the sentence structure for better rhythm.
🔎 Proposed fix
-- **Run tests (unit only):** `vendor/bin/phpunit --filter Test$` (or target specific files) to avoid cluster requirements. -- **Run specific test file:** `vendor/bin/phpunit tests/PriorityClassTest.php`. -- **Run specific test method:** `vendor/bin/phpunit tests/PriorityClassTest.php --filter test_priority_class_build`. +- **Tests (unit only):** `vendor/bin/phpunit --filter Test$` (or target specific files) to avoid cluster requirements. +- **Run a specific test file:** `vendor/bin/phpunit tests/PriorityClassTest.php`. +- **Execute a specific test method:** `vendor/bin/phpunit tests/PriorityClassTest.php --filter test_priority_class_build`.src/Auth/OpenShiftOAuthProvider.php (1)
127-129: Generic exception catch is too broad.Catching
\Exceptioncatches everything including programming errors, type errors, etc. You'll silently fall back to the default pattern even when there's a real code problem. Catch specific exceptions:\GuzzleHttp\Exception\GuzzleExceptionfor HTTP failures, or at minimum\RuntimeExceptionand\JsonException.🔎 Proposed fix
- } catch (\Exception $e) { + } catch (\GuzzleHttp\Exception\GuzzleException|\JsonException $e) { // Fallback to constructing from cluster URL }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
AGENTS.mdUPGRADING.mdsrc/Auth/OpenShiftOAuthProvider.phpsrc/Enums/Protocol.php
🚧 Files skipped from review as they are similar to previous changes (2)
- UPGRADING.md
- src/Enums/Protocol.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/Auth/OpenShiftOAuthProvider.php (3)
src/Exceptions/AuthenticationException.php (1)
AuthenticationException(7-10)src/Auth/TokenProvider.php (2)
TokenProvider(8-55)refresh(54-54)src/Contracts/TokenProviderInterface.php (1)
refresh(22-22)
🪛 LanguageTool
AGENTS.md
[style] ~54-~54: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...les) to avoid cluster requirements. - Run specific test file: `vendor/bin/phpun...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~56-~56: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...-filter test_priority_class_build. - **Run with CI environment:** CI=true vendor/...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~202-~202: Consider a different adjective to strengthen your wording.
Context: ...i/) If anything is unclear or you need deeper context for a change, open a draft PR w...
(DEEP_PROFOUND)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (2)
src/Auth/OpenShiftOAuthProvider.php (2)
75-88: Location header validation is solid.This properly addresses the earlier review comment. The check for
hasHeader(), followed bygetHeaderLine(), followed by empty string validation is the right approach with clear error messages.
132-137: parse_url validation is correct.The validation for
parse_urlreturningfalseor missing thehostkey properly addresses the earlier review concern. Fallback to$this->clusterUrlis reasonable.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/Kinds/VolumeSnapshotContent.php (1)
43-179: Missing explicit return type declarations on spec methods.The status methods at the bottom have proper return types, but all spec getters/setters don't. For a PR pushing PHP 8.2+ modernization, this inconsistency is going to bite you later. The setters should return
static, and getters should match their docblock hints.🔎 Proposed fix for spec methods
- public function setDeletionPolicy(string $policy) + public function setDeletionPolicy(string $policy): static { return $this->setSpec('deletionPolicy', $policy); } - public function getDeletionPolicy() + public function getDeletionPolicy(): ?string { return $this->getSpec('deletionPolicy'); } - public function setDriver(string $driver) + public function setDriver(string $driver): static { return $this->setSpec('driver', $driver); } - public function getDriver() + public function getDriver(): ?string { return $this->getSpec('driver'); } - public function setVolumeSnapshotRef(string $namespace, string $name) + public function setVolumeSnapshotRef(string $namespace, string $name): static { return $this->setSpec('volumeSnapshotRef', [ 'namespace' => $namespace, 'name' => $name, ]); } - public function getVolumeSnapshotRef() + public function getVolumeSnapshotRef(): ?array { return $this->getSpec('volumeSnapshotRef'); } - public function setVolumeSnapshotClassName(string $className) + public function setVolumeSnapshotClassName(string $className): static { return $this->setSpec('volumeSnapshotClassName', $className); } - public function getVolumeSnapshotClassName() + public function getVolumeSnapshotClassName(): ?string { return $this->getSpec('volumeSnapshotClassName'); } - public function setSnapshotHandle(string $handle) + public function setSnapshotHandle(string $handle): static { return $this->setSpec('source.snapshotHandle', $handle); } - public function getSnapshotHandle() + public function getSnapshotHandle(): ?string { return $this->getSpec('source.snapshotHandle'); } - public function setSourceVolumeHandle(string $handle) + public function setSourceVolumeHandle(string $handle): static { return $this->setSpec('source.volumeHandle', $handle); } - public function getSourceVolumeHandle() + public function getSourceVolumeHandle(): ?string { return $this->getSpec('source.volumeHandle'); } - public function setSourceVolumeMode(string $mode) + public function setSourceVolumeMode(string $mode): static { return $this->setSpec('sourceVolumeMode', $mode); } - public function getSourceVolumeMode() + public function getSourceVolumeMode(): ?string { return $this->getSpec('sourceVolumeMode'); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Auth/EksTokenProvider.phpsrc/Auth/OpenShiftOAuthProvider.phpsrc/Auth/ServiceAccountTokenProvider.phpsrc/Traits/Cluster/MakesHttpCalls.phptests/Kinds/VolumeSnapshotContent.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Auth/EksTokenProvider.php
🧰 Additional context used
🧬 Code graph analysis (4)
src/Traits/Cluster/MakesHttpCalls.php (1)
src/Traits/Cluster/AuthenticatesCluster.php (1)
getAuthToken(224-231)
tests/Kinds/VolumeSnapshotContent.php (4)
tests/Kinds/VolumeSnapshotClass.php (4)
setDeletionPolicy(56-59)getDeletionPolicy(66-69)setDriver(36-39)getDriver(46-49)src/Traits/Resource/HasSpec.php (2)
setSpec(13-16)getSpec(34-37)src/Traits/InitializesResources.php (1)
namespace(71-74)src/Traits/Resource/HasStatus.php (1)
getStatus(12-15)
src/Auth/ServiceAccountTokenProvider.php (4)
src/Auth/TokenProvider.php (2)
TokenProvider(8-55)refresh(54-54)src/Traits/Cluster/AuthenticatesCluster.php (1)
getAuthToken(224-231)src/Contracts/TokenProviderInterface.php (1)
refresh(22-22)src/Traits/Cluster/MakesHttpCalls.php (1)
callWithToken(201-219)
src/Auth/OpenShiftOAuthProvider.php (3)
src/Exceptions/AuthenticationException.php (1)
AuthenticationException(7-10)src/Auth/TokenProvider.php (2)
TokenProvider(8-55)refresh(54-54)src/Contracts/TokenProviderInterface.php (1)
refresh(22-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (8)
src/Auth/OpenShiftOAuthProvider.php (4)
23-28: LGTM!Constructor is clean and properly initializes all required properties. The
rtrimon the cluster URL prevents double-slash issues.
30-42: LGTM!Fluent configuration methods are implemented correctly. The
withoutSslVerification()method is appropriately named to make the security implication obvious.
44-117: LGTM! Previous issues addressed.The
refresh()method correctly implements the OpenShift OAuth flow:
- Exception handling now catches
GuzzleExceptionand properly handles both HTTP errors and network failures- Location header validation is thorough (presence + non-empty checks)
- Token extraction and expiration parsing are solid
All three previous review concerns have been properly addressed.
122-157: LGTM! parse_url failure now handled.The discovery logic is solid:
- Attempts well-known endpoint first
- Falls back to OpenShift URL pattern construction
- The
parse_urlvalidation (lines 145-147) correctly addresses the previous review concern- Broad exception catch on line 137 is acceptable since this is discovery with fallback
All previous issues have been resolved.
src/Traits/Cluster/MakesHttpCalls.php (1)
84-87: LGTM - Correct abstraction for token retrieval.Switching from direct token access to
getAuthToken()properly enables the token provider system while maintaining backward compatibility with simple token authentication.tests/Kinds/VolumeSnapshotContent.php (3)
1-36: Class structure looks solid.The class declaration, imports, and static properties follow the established K8sResource pattern correctly. The
$namespaceable = falseis accurate for VolumeSnapshotContent since it's cluster-scoped.
181-187: Good strict comparison.Using
=== trueinstead of a truthy check is the right call sincegetStatus()could returnnull.
197-219: Remaining status methods are correctly typed.
snapshotHandleas?string,creationTimeas?int(nanoseconds since epoch), anderroras?arrayall match the Kubernetes API spec.
cuppett
force-pushed
the
master
branch
3 times, most recently
from
3efa7cd to
72002dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
AGENTS.md (1)
193-193: CI matrix documentation is incorrect.Line 193 states "CI matrix runs PHP 8.3/8.4/8.5" but the actual CI configuration (
.github/workflows/ci.ymllines 24-63) runs PHP 8.2, 8.3, 8.4, AND 8.5. The omission of 8.2 is misleading since it's the minimum supported version per line 43.🔎 Proposed fix
-- CI matrix runs PHP 8.3/8.4/8.5 across Kubernetes v1.32.9, v1.33.5, v1.34.1 and Laravel 11/12 with both `prefer-lowest` and `prefer-stable`. +- CI matrix runs PHP 8.2/8.3/8.4/8.5 across Kubernetes v1.32.9, v1.33.5, v1.34.1 and Laravel 11/12 with both `prefer-lowest` and `prefer-stable`.src/Traits/Cluster/MakesHttpCalls.php (1)
202-220: Make this method private, not protected.The past review correctly identified that this method should be
private, notprotected. The temporary state mutation pattern (lines 205-218) creates a race condition in async contexts—and this library actively uses ReactPHP (Ratchet/Pawl for WebSocket operations per AGENTS.md and codebase).Protected visibility is too broad.
ServiceAccountTokenProvidercan still call this method because it's in the same package (src/Auth/), so there's no reason to expose it beyond this trait. The docblock warning is good but doesn't prevent misuse—private visibility does.🔎 Proposed fix
- protected function callWithToken(string $token, string $method, string $path, string $payload = '', array $query = ['pretty' => 1], array $options = []) + private function callWithToken(string $token, string $method, string $path, string $payload = '', array $query = ['pretty' => 1], array $options = [])src/Kinds/K8sCSIDriver.php (1)
116-124: Past feedback was not actually applied - missing return type ongetFsGroupPolicy().The previous review requested adding a return type to this method, and it was marked as "✅ Addressed in commit 8aaac58", but the code still lacks the return type declaration. Every other getter in this class has an explicit return type, making this inconsistent.
🔎 Proposed fix
- public function getFsGroupPolicy() + public function getFsGroupPolicy(): ?string { return $this->getSpec('fsGroupPolicy'); }
🧹 Nitpick comments (8)
tests/Kinds/VolumeSnapshotContent.php (3)
48-56: Add explicit return type declarations to spec getter methods.The status getters have explicit return types (per previous review feedback), but the spec getters still lack them. For consistency and to complete the PHP 8.2+ modernization, add explicit return types to all spec getters.
🔎 Proposed refactor
/** * Get the deletion policy. - * - * @return string|null */ - public function getDeletionPolicy() + public function getDeletionPolicy(): ?string { return $this->getSpec('deletionPolicy'); }Apply similar changes to:
getDriver(): ?string,getVolumeSnapshotRef(): ?array,getVolumeSnapshotClassName(): ?string,getSnapshotHandle(): ?string,getSourceVolumeHandle(): ?string, andgetSourceVolumeMode(): ?string.Also applies to: 68-76, 91-99, 111-119, 131-139, 151-159, 171-179
189-219: Consider removing redundant docblocks from status getters.Now that explicit return types are present (per previous review), the minimal docblocks on
getRestoreSize,getSnapshotHandleFromStatus,getCreationTime, andgetErrorprovide little additional value. Modern PHP code style often omits docblocks when type hints are sufficient.This is purely a style preference—the code functions correctly either way.
43-46: Adding return type declarations here creates inconsistency with the codebase.The library is already PHP 8.2+, but setter methods are inconsistently typed. While some setters like
ExecCredentialProvider::setClusterInfo()andTokenProvider::setRefreshBuffer()use: static, the vast majority of setters across traits and Kinds classes lack return types. Adding: selfto these seven setters without addressing the broader pattern would be a half-measure that doesn't improve consistency.Either systematically add return types across the entire library's setters, or leave this class as-is with the rest of the codebase.
tests/CSIDriverTest.php (1)
147-165: Consider prefixing unused callback parameters with underscore.Both watch callbacks receive a
$typeparameter that isn't used in the test logic. While this is fine for test code, you can silence static analysis warnings by prefixing with underscore:$_type.🔎 Proposed fix
- public function runWatchAllTests() - { - $watch = $this->cluster->csiDriver()->watchAll(function ($type, $csiDriver) { + $watch = $this->cluster->csiDriver()->watchAll(function ($_type, $csiDriver) { if ($csiDriver->getName() === 'test-csi-driver.example.com') { return true; } }, ['timeoutSeconds' => 10]); - public function runWatchTests() - { - $watch = $this->cluster->csiDriver()->watchByName('test-csi-driver.example.com', function ($type, $csiDriver) { + $watch = $this->cluster->csiDriver()->watchByName('test-csi-driver.example.com', function ($_type, $csiDriver) { return $csiDriver->getName() === 'test-csi-driver.example.com'; }, ['timeoutSeconds' => 10]);src/Auth/TokenProvider.php (1)
28-37: Consider using DateTimeImmutable for buffer calculation.Line 34 creates a mutable
DateTimeobject. Since you're already usingDateTimeImmutableelsewhere in the codebase (e.g., line 99 in ExecCredentialProvider), it's more consistent to use it here as well.🔎 Proposed fix
- $bufferTime = (new \DateTime)->modify("+{$this->refreshBuffer} seconds"); + $bufferTime = (new \DateTimeImmutable)->modify("+{$this->refreshBuffer} seconds");src/Auth/OpenShiftOAuthProvider.php (1)
128-139: No error handling for malformed JSON from well-known endpoint.If the OAuth server returns invalid JSON,
json_decodereturnsnulland$config['issuer']will fail silently (PHP 8+ throws TypeError on null array access). The catch-all\Exceptionon line 137 saves you here, but it's masking the real error. Not a blocker since the fallback works, but you're flying blind on why discovery failed.🔎 Explicit JSON error handling
$config = json_decode($response->getBody(), true); + if (json_last_error() !== JSON_ERROR_NONE) { + throw new \RuntimeException('Invalid JSON from OAuth discovery'); + } if (isset($config['issuer'])) {tests/VolumeAttributesClassTest.php (1)
152-170: Unused$typeparameter in watch callbacks is fine.The static analysis flagged
$typeas unused, but this is the required callback signature for watch operations. You could prefix with underscore ($_type) to signal intentional disuse, but it's not worth the churn for test code.src/KubernetesCluster.php (1)
193-195: Update PHPDoc to match mixed return type.The docblocks on lines 193 and 278 say
@return bool, but the signatures correctly declaremixed. These methods returnnull, callback results, or other values depending on the operation. The code is correct; the docs are stale.🔎 Proposed doc fix
/** * Watch for the current resource or a resource list. * - * @return bool + * @return mixed */ protected function watchPath(string $path, Closure $callback, array $query = ['pretty' => 1]): mixed /** * Watch for the logs for the resource. * - * @return bool + * @return mixed */ protected function watchLogsPath(string $path, Closure $callback, array $query = ['pretty' => 1]): mixedAlso applies to: 278-280
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (64)
.github/dependabot.yml.github/workflows/ci.ymlAGENTS.mdPATCH_SUPPORT.mdUPGRADING.mdcomposer.jsonexamples/patch_examples.phpsrc/Auth/EksTokenProvider.phpsrc/Auth/ExecCredentialProvider.phpsrc/Auth/OpenShiftOAuthProvider.phpsrc/Auth/ServiceAccountTokenProvider.phpsrc/Auth/TokenProvider.phpsrc/Contracts/TokenProviderInterface.phpsrc/Enums/AccessMode.phpsrc/Enums/ConditionStatus.phpsrc/Enums/Operation.phpsrc/Enums/PodPhase.phpsrc/Enums/Protocol.phpsrc/Enums/PullPolicy.phpsrc/Enums/RestartPolicy.phpsrc/Enums/ServiceType.phpsrc/Exceptions/AuthenticationException.phpsrc/Instances/Instance.phpsrc/Instances/ResourceMetric.phpsrc/Kinds/K8sCSIDriver.phpsrc/Kinds/K8sCSINode.phpsrc/Kinds/K8sPod.phpsrc/Kinds/K8sResource.phpsrc/Kinds/K8sScale.phpsrc/Kinds/K8sService.phpsrc/Kinds/K8sVerticalPodAutoscaler.phpsrc/Kinds/K8sVolumeAttributesClass.phpsrc/KubernetesCluster.phpsrc/Patches/JsonMergePatch.phpsrc/Patches/JsonPatch.phpsrc/Traits/Cluster/AuthenticatesCluster.phpsrc/Traits/Cluster/LoadsFromKubeConfig.phpsrc/Traits/Cluster/MakesHttpCalls.phpsrc/Traits/Cluster/MakesWebsocketCalls.phpsrc/Traits/InitializesResources.phpsrc/Traits/RunsClusterOperations.phptests/Auth/EksTokenProviderTest.phptests/Auth/ExecCredentialIntegrationTest.phptests/Auth/ExecCredentialProviderTest.phptests/Auth/OpenShiftOAuthProviderTest.phptests/Auth/ServiceAccountTokenIntegrationTest.phptests/Auth/TokenProviderTest.phptests/CSIDriverTest.phptests/CSINodeTest.phptests/CronJobTest.phptests/JobTest.phptests/Kinds/VolumeSnapshotClass.phptests/Kinds/VolumeSnapshotContent.phptests/KubeConfigTest.phptests/VolumeAttributesClassTest.phptests/VolumeSnapshotClassTest.phptests/VolumeSnapshotContentTest.phptests/cluster/exec-credential.jsontests/yaml/csidriver.yamltests/yaml/csinode.yamltests/yaml/kubeconfig-exec.yamltests/yaml/volumeattributesclass.yamltests/yaml/volumesnapshotclass.yamltests/yaml/volumesnapshotcontent.yaml
💤 Files with no reviewable changes (2)
- examples/patch_examples.php
- PATCH_SUPPORT.md
✅ Files skipped from review due to trivial changes (2)
- tests/yaml/kubeconfig-exec.yaml
- tests/cluster/exec-credential.json
🚧 Files skipped from review as they are similar to previous changes (26)
- src/Enums/RestartPolicy.php
- tests/JobTest.php
- src/Enums/Operation.php
- src/Enums/PodPhase.php
- src/Enums/ServiceType.php
- src/Instances/ResourceMetric.php
- src/Traits/RunsClusterOperations.php
- tests/KubeConfigTest.php
- src/Patches/JsonPatch.php
- src/Traits/Cluster/AuthenticatesCluster.php
- src/Kinds/K8sService.php
- tests/Auth/ServiceAccountTokenIntegrationTest.php
- tests/Auth/OpenShiftOAuthProviderTest.php
- src/Kinds/K8sCSINode.php
- src/Patches/JsonMergePatch.php
- tests/yaml/volumesnapshotclass.yaml
- src/Exceptions/AuthenticationException.php
- src/Traits/InitializesResources.php
- tests/yaml/csidriver.yaml
- tests/Auth/ExecCredentialProviderTest.php
- src/Traits/Cluster/MakesWebsocketCalls.php
- tests/CronJobTest.php
- tests/yaml/volumeattributesclass.yaml
- composer.json
- tests/yaml/csinode.yaml
- .github/dependabot.yml
🧰 Additional context used
🧬 Code graph analysis (18)
src/Kinds/K8sResource.php (2)
src/Patches/JsonMergePatch.php (1)
toJson(129-132)src/Patches/JsonPatch.php (1)
toJson(163-166)
src/Contracts/TokenProviderInterface.php (2)
src/Auth/TokenProvider.php (4)
getToken(19-26)isExpired(28-37)refresh(54-54)getExpiresAt(39-42)tests/Auth/TokenProviderTest.php (2)
refresh(14-18)refresh(25-30)
src/Traits/Cluster/MakesHttpCalls.php (1)
src/Traits/Cluster/AuthenticatesCluster.php (1)
getAuthToken(224-231)
src/Auth/ServiceAccountTokenProvider.php (3)
src/Auth/TokenProvider.php (2)
TokenProvider(8-55)refresh(54-54)src/Traits/Cluster/AuthenticatesCluster.php (1)
getAuthToken(224-231)src/Traits/Cluster/MakesHttpCalls.php (1)
callWithToken(202-220)
src/Auth/ExecCredentialProvider.php (2)
src/Exceptions/AuthenticationException.php (1)
AuthenticationException(7-10)src/Auth/TokenProvider.php (2)
TokenProvider(8-55)refresh(54-54)
tests/VolumeSnapshotClassTest.php (1)
tests/Kinds/VolumeSnapshotClass.php (7)
VolumeSnapshotClass(8-88)setDriver(36-39)setDeletionPolicy(56-59)setParameters(76-79)getDriver(46-49)getDeletionPolicy(66-69)getParameters(84-87)
src/Kinds/K8sVerticalPodAutoscaler.php (1)
src/Traits/Resource/HasSpec.php (1)
setSpec(13-16)
src/Auth/EksTokenProvider.php (4)
src/Exceptions/AuthenticationException.php (1)
AuthenticationException(7-10)src/Auth/TokenProvider.php (2)
TokenProvider(8-55)refresh(54-54)src/Auth/OpenShiftOAuthProvider.php (2)
__construct(23-28)refresh(44-117)src/Contracts/TokenProviderInterface.php (1)
refresh(22-22)
src/Instances/Instance.php (4)
src/Instances/ResourceMetric.php (1)
toArray(142-147)src/Kinds/K8sResource.php (1)
toArray(116-133)src/Patches/JsonMergePatch.php (1)
toArray(119-122)src/Patches/JsonPatch.php (1)
toArray(153-156)
tests/Auth/ExecCredentialIntegrationTest.php (2)
src/Traits/Cluster/LoadsFromKubeConfig.php (2)
fromKubeConfigArray(104-109)fromKubeConfigYamlFile(94-97)src/Traits/Cluster/AuthenticatesCluster.php (1)
getAuthToken(224-231)
tests/VolumeAttributesClassTest.php (8)
src/Kinds/K8sVolumeAttributesClass.php (5)
K8sVolumeAttributesClass(8-68)setDriverName(36-39)setParameters(56-59)getDriverName(46-49)getParameters(64-67)src/ResourcesList.php (1)
ResourcesList(7-10)src/Traits/Resource/HasLabels.php (2)
setLabels(12-15)getLabels(20-23)src/Traits/Resource/HasVersion.php (1)
getApiVersion(39-42)src/K8s.php (1)
fromYamlFile(57-66)src/Traits/Cluster/ChecksClusterVersion.php (1)
newerThan(58-65)src/Traits/Resource/HasAttributes.php (1)
isSynced(116-119)src/Kinds/K8sResource.php (1)
watchByName(173-176)
tests/Auth/EksTokenProviderTest.php (7)
src/Auth/EksTokenProvider.php (5)
EksTokenProvider(7-118)isAvailable(25-29)refresh(45-87)withProfile(31-36)withAssumeRole(38-43)src/Exceptions/AuthenticationException.php (1)
AuthenticationException(7-10)tests/TestCase.php (1)
TestCase(12-199)src/Auth/OpenShiftOAuthProvider.php (1)
refresh(44-117)src/Auth/ServiceAccountTokenProvider.php (1)
refresh(57-104)src/Auth/TokenProvider.php (1)
refresh(54-54)src/Contracts/TokenProviderInterface.php (1)
refresh(22-22)
src/Traits/Cluster/LoadsFromKubeConfig.php (2)
src/Auth/ExecCredentialProvider.php (2)
ExecCredentialProvider(8-106)setClusterInfo(42-47)src/Traits/Cluster/AuthenticatesCluster.php (1)
withTokenProvider(212-218)
tests/Auth/TokenProviderTest.php (2)
src/Auth/TokenProvider.php (6)
TokenProvider(8-55)refresh(54-54)isExpired(28-37)getToken(19-26)setRefreshBuffer(47-52)getExpiresAt(39-42)src/Contracts/TokenProviderInterface.php (4)
refresh(22-22)isExpired(17-17)getToken(12-12)getExpiresAt(27-27)
src/Kinds/K8sCSIDriver.php (2)
src/Kinds/K8sResource.php (1)
K8sResource(23-257)src/Traits/Resource/HasSpec.php (2)
setSpec(13-16)getSpec(34-37)
src/Auth/TokenProvider.php (2)
src/Contracts/TokenProviderInterface.php (4)
getToken(12-12)isExpired(17-17)refresh(22-22)getExpiresAt(27-27)tests/Auth/TokenProviderTest.php (2)
refresh(14-18)refresh(25-30)
src/Kinds/K8sPod.php (3)
src/Traits/Resource/HasSpec.php (2)
setSpec(13-16)getSpec(34-37)src/Traits/Resource/HasStatusPhase.php (1)
getPhase(14-17)src/Enums/PodPhase.php (2)
isSuccessful(45-48)isTerminal(23-29)
tests/Kinds/VolumeSnapshotClass.php (3)
tests/Kinds/VolumeSnapshotContent.php (2)
setDriver(63-66)getDriver(73-76)src/Traits/Resource/HasAttributes.php (2)
setAttribute(44-49)getAttribute(85-88)src/Kinds/K8sVolumeAttributesClass.php (2)
setParameters(56-59)getParameters(64-67)
🪛 LanguageTool
AGENTS.md
[style] ~54-~54: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...les) to avoid cluster requirements. - Run specific test file: `vendor/bin/phpun...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~55-~55: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...punit tests/PriorityClassTest.php. - **Run specific test method:** vendor/bin/php...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~56-~56: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...-filter test_priority_class_build. - **Run with CI environment:** CI=true vendor/...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~202-~202: Consider a different adjective to strengthen your wording.
Context: ...i/) If anything is unclear or you need deeper context for a change, open a draft PR w...
(DEEP_PROFOUND)
🪛 PHPMD (2.15.0)
tests/VolumeAttributesClassTest.php
154-154: Avoid unused parameters such as '$type'. (undefined)
(UnusedFormalParameter)
165-165: Avoid unused parameters such as '$type'. (undefined)
(UnusedFormalParameter)
tests/CSIDriverTest.php
149-149: Avoid unused parameters such as '$type'. (undefined)
(UnusedFormalParameter)
160-160: Avoid unused parameters such as '$type'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (51)
src/Enums/Protocol.php (1)
1-15: Clean implementation.This enum is exactly what it should be—three protocol cases with no unnecessary methods. The previous dead code removal was the right call.
src/Enums/ConditionStatus.php (1)
1-39: Logic is solid.The three helper methods are correctly implemented.
isKnown()properly returns the inverse ofUNKNOWN, and the identity checks forisTrue()/isFalse()are straightforward. This is a clean, useful enum.src/Enums/PullPolicy.php (1)
1-34: Correct implementation.Both helper methods have sound logic—
ALWAYSdisallows caching, whileNEVERandIF_NOT_PRESENTallow it. The match expression inallowsCached()is the right tool here.src/Enums/AccessMode.php (1)
1-46: All three helper methods are correct.The logic accurately reflects Kubernetes persistent volume access modes:
allowsWrite()correctly excludes onlyREAD_ONLY_MANYallowsMultiple()correctly identifies the*_MANYmodesisPodScoped()correctly identifies the pod-scoped variantThis is well-implemented.
tests/yaml/volumesnapshotcontent.yaml (1)
1-14: LGTM!The YAML manifest is well-formed and aligns correctly with the test expectations in VolumeSnapshotContentTest.php. All required fields for a VolumeSnapshotContent resource are present with appropriate values.
tests/VolumeSnapshotContentTest.php (5)
11-34: LGTM!Comprehensive builder test covering all spec fields with clear assertions. The fluent API pattern is properly validated.
36-60: LGTM!The YAML loading test properly validates deserialization of the manifest. The defensive array handling (lines 42-50) ensures test robustness regardless of whether
fromYamlFilereturns a single resource or a collection.
62-80: LGTM!Status method validation is thorough and correctly uses
setAttributefor test setup. The assertions properly verify all status accessors including readiness, size, handle, timestamp, and error information.
82-98: LGTM!The API interaction test properly guards against missing CRD installation (lines 87-91) and delegates to well-structured CRUD helper methods. This ensures tests only run when the cluster environment supports VolumeSnapshot CRDs.
100-181: LGTM!The CRUD helper methods provide comprehensive lifecycle coverage:
- Creation validates sync/exists state transitions
- Get operations validate both listing and retrieval by name
- Updates verify label persistence
- Deletion properly asserts exception on subsequent retrieval
The test structure is clean and follows PHPUnit best practices.
tests/Kinds/VolumeSnapshotContent.php (1)
11-36: LGTM!Class structure correctly defines VolumeSnapshotContent as a cluster-scoped resource using the snapshot.storage.k8s.io/v1 API. The trait composition (HasSpec, HasStatus, HasStatusConditions) is appropriate for this CSI resource type.
.github/workflows/ci.yml (1)
24-63: LGTM—solid CI modernization.The PHP 8.5 matrix entries ensure forward compatibility, the cache v5 upgrade is a sensible infra update, and the GITHUB_TOKEN prevents rate-limit issues with GitHub's minikube downloads. Quote style standardization is harmless. All changes align with the PR's testing objectives.
Also applies to: 93-93, 106-107
src/Kinds/K8sVerticalPodAutoscaler.php (1)
52-55: LGTM—type hints enable fluent chaining.The
: staticreturn types are correct since both methods delegate tosetSpec()which returns$this. This aligns with the PR's type-safety goals and maintains the fluent interface pattern.Also applies to: 60-63
src/Traits/Cluster/LoadsFromKubeConfig.php (1)
221-237: LGTM—clean exec credential integration.The implementation correctly creates an
ExecCredentialProviderfrom kubeconfig, conditionally injects cluster info whenprovideClusterInfois set, and registers it viawithTokenProvider(). This properly integrates modern exec-based authentication into the kubeconfig loader.src/Kinds/K8sScale.php (1)
6-6: LGTM—enum migration maintains scale semantics.The replacement of
KubernetesCluster::REPLACE_OPwithOperation::REPLACEis correct. Scale subresources properly use PUT (replace) operations as documented in the comments, and the enum migration aligns with the PR's modernization goals.Also applies to: 101-101, 124-124
src/Kinds/K8sResource.php (1)
141-144: LGTM—accurate return type for json_encode.The
string|falsereturn type correctly reflectsjson_encode()'s behavior (returnsfalseon failure). This matches the pattern used inJsonPatchandJsonMergePatchclasses and improves type safety.UPGRADING.md (1)
1-191: LGTM—comprehensive upgrade documentation.The upgrade guide clearly documents breaking changes (Operation enum migration, RestartPolicy return type), provides concrete before/after examples, covers new enum features with helper methods, and includes a practical migration checklist. The troubleshooting section addresses common migration pain points. This will help users navigate the PHP 8.2+ modernization smoothly.
src/Contracts/TokenProviderInterface.php (1)
7-27: LGTM!Clean interface design with explicit return types and clear method contracts. The interface properly defines the token provider contract.
tests/CSIDriverTest.php (2)
11-34: LGTM!Comprehensive test coverage for the fluent builder pattern. All CSIDriver properties are properly validated.
36-49: LGTM!YAML loading test correctly validates deserialization of CSIDriver configuration.
src/Instances/Instance.php (1)
23-26: LGTM!Adding explicit return type
arrayimproves type safety and aligns with PHP 8.2+ modernization goals.src/Kinds/K8sVolumeAttributesClass.php (1)
8-68: LGTM!Clean implementation following established K8s resource patterns. Return types are properly declared (past feedback addressed), and the fluent interface is correctly implemented.
tests/Auth/EksTokenProviderTest.php (1)
11-91: LGTM!Well-structured tests that appropriately handle AWS SDK availability. Using reflection to verify internal configuration is acceptable for unit tests, and the conditional skipping ensures tests run in various environments.
src/Auth/ExecCredentialProvider.php (4)
24-40: LGTM!Constructor correctly parses exec configuration, including the Kubernetes-style env array format.
42-47: LGTM!Clean fluent setter for cluster info injection.
49-84: Refresh implementation looks solid.The environment variable merging (line 59) was previously discussed, and the trade-off between functionality (supporting providers like AWS EKS that need ambient credentials) and least-privilege is understood. The error handling with optional install hints is helpful.
86-105: LGTM!Proper JSON parsing with validation and graceful handling of both expiring and non-expiring tokens.
src/Auth/TokenProvider.php (3)
19-26: LGTM!Automatic refresh logic ensures tokens are always fresh when retrieved.
39-52: LGTM!Clean getter and fluent setter implementation.
54-54: LGTM!Proper abstract method declaration that subclasses must implement.
src/Kinds/K8sCSIDriver.php (1)
1-179: Overall implementation is solid except for the missing return type.The class properly extends K8sResource, implements the required interfaces, and provides a comprehensive fluent API for CSI driver configuration. All setters and getters follow the established patterns (aside from the
getFsGroupPolicy()return type issue).src/Auth/OpenShiftOAuthProvider.php (3)
62-76: Exception handling looks solid now.The broad
GuzzleExceptioncatch with the nestedRequestExceptioncheck handles network errors, timeouts, and HTTP errors gracefully. The null response check prevents accessing methods on null.
85-98: Location header validation is thorough.The three-tier validation (header exists, header not empty, contains access_token) covers all edge cases properly. This directly addresses previous concerns about missing headers.
141-157: Fallback URL construction handles edge cases properly.The
parse_urlvalidation and regex-based transformation for common OpenShift patterns is pragmatic. Returning$this->clusterUrlas the ultimate fallback is sensible—better to try something than fail silently.tests/Kinds/VolumeSnapshotClass.php (1)
8-88: Test resource class correctly models VolumeSnapshotClass CRD structure.The use of
setAttribute(notsetSpec) fordriver,deletionPolicy, andparametersis correct—these are top-level fields in the VolumeSnapshotClass CRD, not nested underspec. The implementation mirrors the pattern used inK8sVolumeAttributesClassfrom the relevant snippets.tests/VolumeAttributesClassTest.php (2)
134-150: Deletion polling loop is a good pattern.The timeout-based polling for deletion completion prevents flaky tests. The 60-second timeout with 2-second intervals is reasonable for CI environments.
47-50: The version check is correct—no changes needed.VolumeAttributesClass graduated to GA in Kubernetes v1.34, released September 2025. The code correctly checks for version 1.34.0. Changing it to 1.31 would allow tests to run on unsupported versions where the feature doesn't exist in GA form.
Likely an incorrect or invalid review comment.
tests/CSINodeTest.php (2)
83-106: Pragmatic handling of optional CSI driver presence.Using
addWarninginstead of failing when no CSINodes exist is the right call—CSINode presence depends on cluster configuration, not code correctness. The nested driver checks also handle the case where a node exists but has no drivers yet.
52-76: Driver method test coverage is thorough.Testing both existing and nonexistent driver scenarios with proper null/empty-array expectations catches edge cases that often slip through. The assertions on
hasDriver,getDriverByName,getNodeIdForDriver,getTopologyKeysForDriver, andgetAllocatableForDrivercover the full API surface.tests/Auth/TokenProviderTest.php (3)
8-31: Test doubles are well-designed.
MockTokenProvidergives full control over token/expiration for testing edge cases.CountingTokenProvidercleanly verifies thatgetToken()triggers refresh only when needed. These are textbook test doubles—minimal, focused, no unnecessary logic.
61-68: Buffer boundary test nails the edge case.Testing that a token expiring in 30 seconds is considered expired with a 60-second buffer directly validates the "refresh before actual expiration" behavior. This is the kind of test that catches off-by-one bugs in time comparisons.
70-82: Lazy refresh verification is solid.Checking that
refreshCountis 1 after two consecutivegetToken()calls proves the caching works. TheassertEquals($token1, $token2)further confirms the same token is returned. Clean test.src/Kinds/K8sPod.php (3)
212-239: Enum-based restart policy API is a clean modernization.Using
RestartPolicy::ON_FAILURE->valuefor storage while exposingRestartPolicyenum in the getter is the right pattern—internal storage stays compatible with Kubernetes API, but consumers get type safety. TheRestartPolicy::from()with a default handles missing values gracefully.This is a breaking change for
getRestartPolicy()callers expecting a string, which is documented in UPGRADING.md per the PR objectives.
427-459: PodPhase enum integration is well-implemented.The
getPodPhase()method handles null phases by falling back toUNKNOWN, which is defensive and correct—pods can have no phase early in their lifecycle. DelegatingisTerminal()to the enum's method is clean separation of concerns.One nit: the ternary
$phase ?: PodPhase::UNKNOWN->valueis clever but subtle. If$phaseis an empty string (whichgetPhase()could theoretically return), it would also fall back to UNKNOWN. That's probably fine, but worth knowing.
440-451: Consistent enum comparisons in isRunning/isSuccessful.Both methods now use
$this->getPodPhase() === PodPhase::*instead of string comparisons. This is more robust against typos and makes the intent explicit. Good modernization.tests/VolumeSnapshotClassTest.php (1)
54-70: CRD availability check is pragmatic.Using a try-catch on
all()to detect missing CRDs is better than hardcoding version checks. The skip message is clear about why the test was skipped.tests/Auth/ExecCredentialIntegrationTest.php (2)
85-118: Test pattern is solid. Usingprintenvto echo back the JSON credential from an env var is a clean way to test the env var passing mechanism without external dependencies. This works fine on the project's Linux-only CI.
30-68: The CI workflow explicitly sets upkubectl proxyon port 8080 before running tests (line 121-123 of.github/workflows/ci.yml). The test is correctly designed as an integration test—thesetUp()method skips it if not in CI and the cluster is unavailable. This is not a flaky test; it's a properly gated integration test that runs only in controlled CI environments where the prerequisites are guaranteed.Likely an incorrect or invalid review comment.
src/Auth/ServiceAccountTokenProvider.php (1)
106-118: Clean recursion prevention pattern.The
makeBootstrapRequest()method correctly uses the captured bootstrap token to avoid infinite recursion when the provider is configured on the same cluster. The return type is properly declared, and the implementation is straightforward.src/KubernetesCluster.php (1)
171-187: Excellent enum migration with backward compatibility.The refactor maintains backward compatibility by accepting
Operation|stringand usingtryFrom()with a GET fallback (line 175). This preserves the previous behavior where unknown operations defaulted to GET, preventing breaking changes. The match expression is clean and type-safe.src/Auth/EksTokenProvider.php (1)
47-51: The AWS SDK dependency is already declared in composer.json as a suggested dependency with the description "Required for native EKS token generation without AWS CLI." No changes needed—this is the correct approach since EKS token generation is optional functionality that users only need if they explicitly use this provider.Likely an incorrect or invalid review comment.
) Updates the requirements on [orchestra/testbench](https://github.com/orchestral/testbench) to permit the latest version. - [Release notes](https://github.com/orchestral/testbench/releases) - [Changelog](https://github.com/orchestral/testbench/blob/10.x/CHANGELOG-10.x.md) - [Commits](https://github.com/orchestral/testbench/commits/v10.8.0) --- updated-dependencies: - dependency-name: orchestra/testbench dependency-version: 10.8.0 dependency-type: direct:development ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [actions/cache](https://github.com/actions/cache) from 4 to 5. - [Release notes](https://github.com/actions/cache/releases) - [Changelog](https://github.com/actions/cache/blob/main/RELEASES.md) - [Commits](actions/cache@v4...v5) --- updated-dependencies: - dependency-name: actions/cache dependency-version: '5' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Updates the requirements on [vimeo/psalm](https://github.com/vimeo/psalm) to permit the latest version. - [Release notes](https://github.com/vimeo/psalm/releases) - [Commits](vimeo/psalm@6.13.1...6.14.2) --- updated-dependencies: - dependency-name: vimeo/psalm dependency-version: 6.14.2 dependency-type: direct:development ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/KubernetesCluster.php (1)
275-280: Same PHPDoc mismatch here.
@return boolin the docblock, but the method returnsmixed. Same fix needed.🔎 Proposed fix
/** * Watch for the logs for the resource. - * - * @return bool */ protected function watchLogsPath(string $path, Closure $callback, array $query = ['pretty' => 1]): mixed
♻️ Duplicate comments (8)
AGENTS.md (1)
193-193: Documentation says PHP 8.3/8.4/8.5 but CI matrix includes 8.2.The CI workflow at line 24 specifies
php: ['8.2', '8.3', '8.4']plus the PHP 8.5 includes. This line should read "PHP 8.2/8.3/8.4/8.5" to match reality.🔎 Proposed fix
-- CI matrix runs PHP 8.3/8.4/8.5 across Kubernetes v1.32.9, v1.33.5, v1.34.1 and Laravel 11/12 with both `prefer-lowest` and `prefer-stable`. +- CI matrix runs PHP 8.2/8.3/8.4/8.5 across Kubernetes v1.32.9, v1.33.5, v1.34.1 and Laravel 11/12 with both `prefer-lowest` and `prefer-stable`.src/Traits/Cluster/MakesHttpCalls.php (1)
190-220: Visibility isprotected, notprivateas previously suggested — verify this is intentional.The past review flagged this method as
publicand recommendedprivate. It's nowprotected, which is better but still allows subclass access. IfServiceAccountTokenProviderneeds to call this from within the same trait composition,protectedmakes sense. The concurrency warning (lines 194-196) is present as recommended.The save-mutate-restore pattern remains inherently unsafe for concurrent async contexts (ReactPHP event loops), but the prominent docblock warning covers this. Just be aware this is a "works for synchronous use" pattern, not a real fix for concurrency.
tests/CSIDriverTest.php (1)
158-165: Same unused parameter issue in watch callback.Same as the previous watch test—
$typeis received but never used.src/Auth/ExecCredentialProvider.php (1)
49-84: Environment variable exposure remains a security risk (revisiting past comment).The
array_merge($_ENV, $_SERVER, $this->env)on line 59 still exposes ALL environment variables to the child process. While your point about needing AWS credentials is valid, this approach is overly broad. In environments with multiple services/secrets, you're potentially leaking database passwords, internal API keys, or other sensitive data to an external binary.The previous suggestion of an allowList is the right path forward. At minimum, document this behavior prominently so users understand the security implications.
Additional concern: Line 56 uses
Process::fromShellCommandline()which interprets shell syntax. While line 53 usesescapeshellarg()for arguments, if$this->commanditself contains shell metacharacters from untrusted input, you'd have command injection. Ensurecommandcomes from trusted config only (which it should, from kubeconfig).src/Kinds/K8sCSINode.php (3)
42-58: Missing return type declaration (past review comment not addressed).The previous review correctly identified that
getDriverByName()lacks a return type declaration. The docblock says@return array|nullbut the method signature doesn't enforce it. This is inconsistent with methods likegetDrivers()andgetTopologyKeysForDriver()which properly declare their return types.Add the return type:
🔎 Proposed fix
/** * Get a specific driver by name. * * @return array|null */ - public function getDriverByName(string $driverName) + public function getDriverByName(string $driverName): ?array { $drivers = $this->getDrivers(); foreach ($drivers as $driver) { if (isset($driver['name']) && $driver['name'] === $driverName) { return $driver; } } return null; }
68-78: Missing return type declaration (past review comment not addressed).Same issue:
getNodeIdForDriver()has@return string|nullin the docblock but no return type on the method signature.🔎 Proposed fix
/** * Get the node ID for a specific driver. * * @return string|null */ - public function getNodeIdForDriver(string $driverName) + public function getNodeIdForDriver(string $driverName): ?string { $driver = $this->getDriverByName($driverName); return $driver['nodeID'] ?? null; }
90-100: Missing return type declaration (past review comment not addressed).Third instance:
getAllocatableForDriver()also lacks the return type declaration matching its@return array|nulldocblock.🔎 Proposed fix
/** * Get allocatable volume resources for a specific driver. * * @return array|null */ - public function getAllocatableForDriver(string $driverName) + public function getAllocatableForDriver(string $driverName): ?array { $driver = $this->getDriverByName($driverName); return $driver['allocatable'] ?? null; }src/KubernetesCluster.php (1)
191-195: PHPDoc lies about return type.The docblock says
@return boolbut the method signature ismixedand the implementation returnsnullor whatever the callback returns - never a boolean. This was flagged before but still not fixed.Either update the docblock to
@return mixedor just remove it since the signature already declaresmixed.🔎 Proposed fix
/** * Watch for the current resource or a resource list. - * - * @return bool */ protected function watchPath(string $path, Closure $callback, array $query = ['pretty' => 1]): mixed
🧹 Nitpick comments (8)
src/Instances/Instance.php (1)
23-26: Type declaration looks good, but patch classes are inconsistent.This is fine. However,
JsonMergePatch::toArray()andJsonPatch::toArray()(per the relevant code snippets) still lack return type declarations. If you're doing a modernization pass, those should get the same treatment for consistency.src/Kinds/K8sVolumeAttributesClass.php (1)
36-39: Add return type declarations to fluent setters for consistency.Given this PR's theme of PHP 8.2+ modernization with comprehensive type hints, the setters should declare their return types. PHP 8.0+ supports
: staticfor fluent interfaces.🔎 Proposed fix
- public function setDriverName(string $driver) + public function setDriverName(string $driver): static { return $this->setAttribute('driverName', $driver); }- public function setParameters(array $parameters) + public function setParameters(array $parameters): static { return $this->setAttribute('parameters', $parameters); }Also applies to: 54-57
tests/Auth/EksTokenProviderTest.php (4)
11-24: Incomplete test coverage for SDK unavailability.This test only verifies the "happy path" when AWS SDK is available. It doesn't test the negative case: when the SDK is missing, you're not verifying that
isAvailable()returnsfalse. The conditional check on line 20 means half your test only runs sometimes, and the false case is never explicitly verified.🔎 Strengthen test coverage
public function test_is_available_checks_aws_sdk() { - // This test checks if AWS SDK detection works $isAvailable = EksTokenProvider::isAvailable(); - // Should be boolean $this->assertIsBool($isAvailable); - // If available, the classes should exist if ($isAvailable) { $this->assertTrue(class_exists(\Aws\Sts\StsClient::class)); $this->assertTrue(class_exists(\Aws\Credentials\CredentialProvider::class)); + } else { + $this->assertFalse(class_exists(\Aws\Sts\StsClient::class)); + $this->assertFalse(class_exists(\Aws\Credentials\CredentialProvider::class)); } }
41-53: This test doesn't test anything meaningful.The method is named
test_eks_token_format()but all it does is instantiate the class and check it's an instance of itself. You're not testing token format, you're not testing behavior—you're just verifying thatnew EksTokenProvider(...)doesn't explode. That's the bare minimum and provides zero regression protection.Either remove this test or add actual token format validation (even if mocked), otherwise it's just dead weight inflating your test count.
55-75: Reflection-based testing is brittle and tests implementation, not behavior.You're reaching into private properties to verify configuration was stored. This couples the test to implementation details. If you rename
$profileto$awsProfile, this test breaks even though the public API is unchanged.Tests should verify observable behavior, not internal state. If these configurations matter, test that they affect token generation (even with mocks).
77-90: Another reflection-based test checking private implementation.Same problem as the previous test. You're verifying a private field's default value instead of testing observable behavior. The default TTL should be validated by checking the actual token expiration after generation, not by peeking at internal state.
tests/CSIDriverTest.php (1)
147-156: Unused callback parameter$type(static analysis hint).The watch callback receives
($type, $csiDriver)but only uses$csiDriver. The$typeparameter (typically 'ADDED', 'MODIFIED', 'DELETED') is unused. This is fine if you don't need it, but it clutters the signature.Consider prefixing with underscore to indicate it's intentionally unused:
function ($type, $csiDriver)→function ($_type, $csiDriver)or just remove it if the API allows.tests/CSINodeTest.php (1)
78-106: Test indirection is unnecessary.
test_csi_node_api_interaction()just callsrunGetAllTests(). Either inline the logic or makerunGetAllTests()the test method itself. The extra layer adds no value.🔎 Proposed simplification
- public function test_csi_node_api_interaction() - { - $this->runGetAllTests(); - } - - public function runGetAllTests() + public function test_csi_node_api_interaction() { // CSINodes are created by kubelet, so we just test listing them $csiNodes = $this->cluster->getAllCSINodes(); // ... rest of the method }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (64)
.github/dependabot.yml.github/workflows/ci.ymlAGENTS.mdPATCH_SUPPORT.mdUPGRADING.mdcomposer.jsonexamples/patch_examples.phpsrc/Auth/EksTokenProvider.phpsrc/Auth/ExecCredentialProvider.phpsrc/Auth/OpenShiftOAuthProvider.phpsrc/Auth/ServiceAccountTokenProvider.phpsrc/Auth/TokenProvider.phpsrc/Contracts/TokenProviderInterface.phpsrc/Enums/AccessMode.phpsrc/Enums/ConditionStatus.phpsrc/Enums/Operation.phpsrc/Enums/PodPhase.phpsrc/Enums/Protocol.phpsrc/Enums/PullPolicy.phpsrc/Enums/RestartPolicy.phpsrc/Enums/ServiceType.phpsrc/Exceptions/AuthenticationException.phpsrc/Instances/Instance.phpsrc/Instances/ResourceMetric.phpsrc/Kinds/K8sCSIDriver.phpsrc/Kinds/K8sCSINode.phpsrc/Kinds/K8sPod.phpsrc/Kinds/K8sResource.phpsrc/Kinds/K8sScale.phpsrc/Kinds/K8sService.phpsrc/Kinds/K8sVerticalPodAutoscaler.phpsrc/Kinds/K8sVolumeAttributesClass.phpsrc/KubernetesCluster.phpsrc/Patches/JsonMergePatch.phpsrc/Patches/JsonPatch.phpsrc/Traits/Cluster/AuthenticatesCluster.phpsrc/Traits/Cluster/LoadsFromKubeConfig.phpsrc/Traits/Cluster/MakesHttpCalls.phpsrc/Traits/Cluster/MakesWebsocketCalls.phpsrc/Traits/InitializesResources.phpsrc/Traits/RunsClusterOperations.phptests/Auth/EksTokenProviderTest.phptests/Auth/ExecCredentialIntegrationTest.phptests/Auth/ExecCredentialProviderTest.phptests/Auth/OpenShiftOAuthProviderTest.phptests/Auth/ServiceAccountTokenIntegrationTest.phptests/Auth/TokenProviderTest.phptests/CSIDriverTest.phptests/CSINodeTest.phptests/CronJobTest.phptests/JobTest.phptests/Kinds/VolumeSnapshotClass.phptests/Kinds/VolumeSnapshotContent.phptests/KubeConfigTest.phptests/VolumeAttributesClassTest.phptests/VolumeSnapshotClassTest.phptests/VolumeSnapshotContentTest.phptests/cluster/exec-credential.jsontests/yaml/csidriver.yamltests/yaml/csinode.yamltests/yaml/kubeconfig-exec.yamltests/yaml/volumeattributesclass.yamltests/yaml/volumesnapshotclass.yamltests/yaml/volumesnapshotcontent.yaml
💤 Files with no reviewable changes (2)
- examples/patch_examples.php
- PATCH_SUPPORT.md
🚧 Files skipped from review as they are similar to previous changes (24)
- src/Enums/ServiceType.php
- .github/dependabot.yml
- tests/yaml/volumesnapshotcontent.yaml
- src/Patches/JsonMergePatch.php
- src/Kinds/K8sResource.php
- src/Patches/JsonPatch.php
- src/Traits/RunsClusterOperations.php
- src/Contracts/TokenProviderInterface.php
- tests/KubeConfigTest.php
- src/Enums/Protocol.php
- src/Kinds/K8sCSIDriver.php
- src/Enums/Operation.php
- tests/yaml/volumesnapshotclass.yaml
- src/Enums/PodPhase.php
- src/Kinds/K8sVerticalPodAutoscaler.php
- tests/Auth/OpenShiftOAuthProviderTest.php
- tests/JobTest.php
- tests/yaml/volumeattributesclass.yaml
- src/Auth/OpenShiftOAuthProvider.php
- src/Enums/PullPolicy.php
- tests/yaml/csidriver.yaml
- tests/yaml/kubeconfig-exec.yaml
- src/Traits/Cluster/LoadsFromKubeConfig.php
- src/Auth/EksTokenProvider.php
🧰 Additional context used
🧬 Code graph analysis (20)
tests/CronJobTest.php (2)
src/Traits/InitializesResources.php (1)
pod(192-195)src/Kinds/K8sPod.php (1)
getRestartPolicy(236-239)
tests/Auth/ExecCredentialIntegrationTest.php (2)
src/Traits/Cluster/LoadsFromKubeConfig.php (2)
fromKubeConfigArray(104-109)fromKubeConfigYamlFile(94-97)src/Traits/Cluster/AuthenticatesCluster.php (1)
getAuthToken(223-230)
src/Traits/Cluster/MakesHttpCalls.php (1)
src/Traits/Cluster/AuthenticatesCluster.php (1)
getAuthToken(223-230)
src/Traits/Cluster/MakesWebsocketCalls.php (1)
src/Traits/Cluster/AuthenticatesCluster.php (1)
getAuthToken(223-230)
src/Instances/Instance.php (4)
src/Instances/ResourceMetric.php (1)
toArray(142-147)src/Kinds/K8sResource.php (1)
toArray(116-133)src/Patches/JsonMergePatch.php (1)
toArray(119-122)src/Patches/JsonPatch.php (1)
toArray(153-156)
tests/Auth/ExecCredentialProviderTest.php (4)
src/Auth/ExecCredentialProvider.php (3)
ExecCredentialProvider(8-106)setClusterInfo(42-47)refresh(49-84)src/Exceptions/AuthenticationException.php (1)
AuthenticationException(7-10)src/Auth/TokenProvider.php (4)
getToken(19-26)getExpiresAt(39-42)isExpired(28-37)refresh(54-54)src/Contracts/TokenProviderInterface.php (4)
getToken(12-12)getExpiresAt(27-27)isExpired(17-17)refresh(22-22)
tests/Auth/ServiceAccountTokenIntegrationTest.php (6)
src/Auth/ServiceAccountTokenProvider.php (3)
ServiceAccountTokenProvider(9-135)withExpirationSeconds(51-56)withAudiences(58-63)src/Exceptions/KubernetesAPIException.php (1)
KubernetesAPIException(5-8)src/KubernetesCluster.php (1)
KubernetesCluster(129-498)src/Auth/TokenProvider.php (2)
getToken(19-26)getExpiresAt(39-42)src/Contracts/TokenProviderInterface.php (2)
getToken(12-12)getExpiresAt(27-27)src/Traits/Cluster/AuthenticatesCluster.php (3)
fromUrl(64-67)withServiceAccountToken(269-288)getAuthToken(223-230)
src/Auth/ExecCredentialProvider.php (3)
src/Exceptions/AuthenticationException.php (1)
AuthenticationException(7-10)src/Auth/TokenProvider.php (2)
TokenProvider(8-55)refresh(54-54)src/Contracts/TokenProviderInterface.php (1)
refresh(22-22)
tests/VolumeAttributesClassTest.php (6)
src/Kinds/K8sVolumeAttributesClass.php (5)
K8sVolumeAttributesClass(8-66)setDriverName(36-39)setParameters(54-57)getDriverName(44-47)getParameters(62-65)src/Traits/InitializesResources.php (1)
volumeAttributesClass(181-184)src/Traits/Resource/HasVersion.php (1)
getApiVersion(39-42)src/Traits/Resource/HasAttributes.php (1)
isSynced(116-119)src/Traits/RunsClusterOperations.php (3)
delete(255-282)watch(407-423)watchAll(382-398)src/Kinds/K8sResource.php (1)
watchByName(173-176)
src/Kinds/K8sCSINode.php (1)
src/Traits/Resource/HasSpec.php (1)
getSpec(34-37)
tests/Auth/TokenProviderTest.php (2)
src/Auth/TokenProvider.php (6)
TokenProvider(8-55)refresh(54-54)isExpired(28-37)getToken(19-26)setRefreshBuffer(47-52)getExpiresAt(39-42)src/Contracts/TokenProviderInterface.php (4)
refresh(22-22)isExpired(17-17)getToken(12-12)getExpiresAt(27-27)
src/Kinds/K8sPod.php (3)
src/Traits/Resource/HasSpec.php (2)
setSpec(13-16)getSpec(34-37)src/Traits/Resource/HasStatusPhase.php (1)
getPhase(14-17)src/Enums/PodPhase.php (2)
isSuccessful(45-48)isTerminal(23-29)
src/Auth/TokenProvider.php (2)
src/Contracts/TokenProviderInterface.php (4)
getToken(12-12)isExpired(17-17)refresh(22-22)getExpiresAt(27-27)tests/Auth/TokenProviderTest.php (2)
refresh(14-18)refresh(25-30)
src/Kinds/K8sService.php (6)
src/Kinds/K8sPod.php (1)
getClusterDns(48-53)src/Contracts/Dnsable.php (1)
getClusterDns(12-12)src/Traits/Resource/HasName.php (1)
getName(36-39)src/Traits/Resource/HasNamespace.php (1)
getNamespace(73-76)src/Traits/Resource/HasSpec.php (3)
setSpec(13-16)getSpec(34-37)addToSpec(24-27)src/Enums/ServiceType.php (1)
isExternallyAccessible(20-26)
tests/VolumeSnapshotClassTest.php (5)
tests/Kinds/VolumeSnapshotClass.php (7)
VolumeSnapshotClass(8-88)setDriver(36-39)setDeletionPolicy(56-59)setParameters(76-79)getDriver(46-49)getDeletionPolicy(66-69)getParameters(84-87)src/Traits/Resource/HasLabels.php (2)
setLabels(12-15)getLabels(20-23)src/Traits/Resource/HasVersion.php (1)
getApiVersion(39-42)src/Traits/Resource/HasAttributes.php (1)
isSynced(116-119)src/Kinds/K8sResource.php (1)
getByName(105-108)
src/Auth/ServiceAccountTokenProvider.php (5)
src/Exceptions/AuthenticationException.php (1)
AuthenticationException(7-10)src/Auth/TokenProvider.php (2)
TokenProvider(8-55)refresh(54-54)src/Traits/Cluster/AuthenticatesCluster.php (1)
getAuthToken(223-230)src/Contracts/TokenProviderInterface.php (1)
refresh(22-22)src/Traits/Cluster/MakesHttpCalls.php (1)
callWithToken(202-220)
src/Traits/Cluster/AuthenticatesCluster.php (5)
src/Auth/TokenProvider.php (1)
getToken(19-26)src/Contracts/TokenProviderInterface.php (1)
getToken(12-12)src/Auth/EksTokenProvider.php (1)
EksTokenProvider(7-129)src/Auth/OpenShiftOAuthProvider.php (2)
OpenShiftOAuthProvider(9-158)withoutSslVerification(37-42)src/Auth/ServiceAccountTokenProvider.php (3)
ServiceAccountTokenProvider(9-135)withExpirationSeconds(51-56)withAudiences(58-63)
tests/Kinds/VolumeSnapshotContent.php (4)
tests/Kinds/VolumeSnapshotClass.php (4)
setDeletionPolicy(56-59)getDeletionPolicy(66-69)setDriver(36-39)getDriver(46-49)src/Traits/Resource/HasSpec.php (2)
setSpec(13-16)getSpec(34-37)src/Traits/InitializesResources.php (1)
namespace(71-74)src/Traits/Resource/HasStatus.php (1)
getStatus(12-15)
src/Traits/InitializesResources.php (3)
src/Kinds/K8sCSIDriver.php (1)
K8sCSIDriver(9-179)src/Kinds/K8sCSINode.php (1)
K8sCSINode(9-101)src/Kinds/K8sVolumeAttributesClass.php (1)
K8sVolumeAttributesClass(8-66)
tests/CSINodeTest.php (4)
src/Kinds/K8sCSINode.php (7)
K8sCSINode(9-101)getDrivers(37-40)hasDriver(63-66)getDriverByName(47-58)getNodeIdForDriver(73-78)getTopologyKeysForDriver(83-88)getAllocatableForDriver(95-100)src/ResourcesList.php (1)
ResourcesList(7-10)src/Traits/Resource/HasSpec.php (1)
setSpec(13-16)src/Traits/Resource/HasVersion.php (1)
getApiVersion(39-42)
🪛 LanguageTool
AGENTS.md
[style] ~54-~54: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...les) to avoid cluster requirements. - Run specific test file: `vendor/bin/phpun...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~55-~55: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...punit tests/PriorityClassTest.php. - **Run specific test method:** vendor/bin/php...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~56-~56: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...-filter test_priority_class_build. - **Run with CI environment:** CI=true vendor/...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~202-~202: Consider a different adjective to strengthen your wording.
Context: ...i/) If anything is unclear or you need deeper context for a change, open a draft PR w...
(DEEP_PROFOUND)
🪛 PHPMD (2.15.0)
tests/VolumeAttributesClassTest.php
154-154: Avoid unused parameters such as '$type'. (undefined)
(UnusedFormalParameter)
165-165: Avoid unused parameters such as '$type'. (undefined)
(UnusedFormalParameter)
tests/CSIDriverTest.php
149-149: Avoid unused parameters such as '$type'. (undefined)
(UnusedFormalParameter)
160-160: Avoid unused parameters such as '$type'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (69)
tests/Kinds/VolumeSnapshotContent.php (1)
1-220: LGTM! Clean implementation with proper type safety.The VolumeSnapshotContent resource class is well-structured with explicit return types throughout. The status accessor methods correctly return nullable types (?string, ?int, ?array) that align with test expectations and the Kubernetes CSI snapshot spec.
tests/Kinds/VolumeSnapshotClass.php (1)
1-88: LGTM! Straightforward resource implementation.The VolumeSnapshotClass resource class is clean and follows the same patterns as other CSI resources in this PR. Return types are appropriate, and the fluent interface is consistent.
tests/VolumeSnapshotClassTest.php (1)
1-150: LGTM! This is the reference implementation.VolumeSnapshotClassTest properly incorporates all the fixes from past reviews: no dead array handling code (line 34-35 has the correct comment), and deletion polling is in place (lines 139-144). This test file demonstrates the correct patterns that VolumeSnapshotContentTest should follow.
.github/workflows/ci.yml (2)
24-63: Solid matrix expansion for PHP 8.5 coverage.The explicit
includeentries for PHP 8.5 paired only with Laravel 12 are the right call—Laravel 11 doesn't officially support 8.5. The coverage across all three Kubernetes versions with bothprefer-lowestandprefer-stableis thorough.
106-107: Good addition of GITHUB_TOKEN for Minikube setup.This prevents rate-limiting issues when Minikube fetches assets from GitHub during provisioning. Smart defensive move.
src/Enums/ConditionStatus.php (1)
10-38: Clean enum implementation matching Kubernetes condition status values.The string values (
'True','False','Unknown') correctly match the Kubernetes API convention. Helper methods are appropriately named and do exactly what they claim.src/Instances/ResourceMetric.php (1)
142-147: Return type declaration aligns with parent class and modernization goals.Matches the parent
Instance::toArray(): arraysignature. Removing the redundant@returndocblock is the right call.src/Enums/AccessMode.php (1)
10-45: Enum correctly models Kubernetes PV access modes with exhaustive match expressions.All four cases are covered in each
match, so noUnhandledMatchErrorrisk when new cases are added (compiler will complain). The boolean logic accurately reflects the Kubernetes semantics.src/Kinds/K8sScale.php (3)
6-6: Clean import for the Operation enum migration.
96-106: Correct use of Operation::REPLACE for scale subresource.Scale subresources don't support POST—only PUT. The docblock makes this clear, and the enum usage aligns with the codebase-wide migration.
116-133: Update method correctly uses REPLACE operation.Consistent with
create()and the scale subresource semantics. The refresh-before-update pattern ensures optimistic locking via resourceVersion.tests/yaml/csinode.yaml (1)
5-13: This review comment is incorrect—CSINode spec.drivers is the correct structure.The Kubernetes CSINode resource uses
spec.drivers, not a top-leveldriversfield. The official Kubernetes API docs explicitly definespec.driversas the location for the drivers array. The YAML in this file is already correct and doesn't need the proposed changes.Likely an incorrect or invalid review comment.
tests/CronJobTest.php (1)
7-7: LGTM — Enum assertions are correct.The test assertions properly compare against
RestartPolicy::NEVERwhich matches the return type ofK8sPod::getRestartPolicy()(returnsRestartPolicy::from(...)). All three test methods are updated consistently.Also applies to: 44-44, 66-66, 124-124
src/Exceptions/AuthenticationException.php (1)
1-10: LGTM — Simple, purpose-built exception.Empty exception classes are a valid pattern for type-specific catching. The providers documented in the summary (
EksTokenProvider,ExecCredentialProvider, etc.) will throw this, allowing consumers to catch authentication failures specifically.tests/cluster/exec-credential.json (1)
1-8: LGTM — Well-structured test fixture.Valid Kubernetes
ExecCredentialformat with a far-future expiration (2099) that won't cause flaky test failures. The placeholder token is clearly identifiable as test data.src/Traits/Cluster/MakesWebsocketCalls.php (2)
51-56: LGTM — Centralized auth token retrieval.Both
getWsClient()andbuildStreamContextOptions()now correctly usegetAuthToken()which returns the token from the provider (if set) or the static token. The fallback to basic auth when no token exists is preserved. This aligns with the HTTP call changes inMakesHttpCalls.php.Also applies to: 107-112
79-81: PHPDoc update is accurate.
fopen()returnsresource|false, so@return false|resourcecorrectly documents the possible return types.src/Enums/RestartPolicy.php (1)
1-31: LGTM — Clean, idiomatic enum.Values correctly map to Kubernetes API strings. The helper methods (
allowsRestarts(),onlyOnFailure()) provide useful convenience without over-engineering. String-backed enum enables seamlessRestartPolicy::from($apiValue)calls.src/Traits/Cluster/MakesHttpCalls.php (1)
84-87: LGTM — Centralized token retrieval for HTTP calls.Using
getAuthToken()correctly prioritizes the token provider over the static token, consistent with the websocket changes.UPGRADING.md (1)
1-191: Solid upgrade guide — addresses the breaking changes clearly.The documentation covers migration paths for enum changes with practical before/after examples. The previous confusing PHP version wording was fixed. Migration checklist and troubleshooting sections are helpful.
src/Kinds/K8sVolumeAttributesClass.php (1)
1-66: LGTM — Clean resource implementation.The class correctly models the Kubernetes
VolumeAttributesClassresource (cluster-scoped,storage.k8s.io/v1). The getter return types are properly declared, and the past review concern aboutgetDriverName()was addressed.tests/Auth/EksTokenProviderTest.php (1)
26-39: Test only runs in environments without AWS SDK.This test is environment-dependent and will be skipped in most local development setups where developers have the AWS SDK installed. That means it's only validated in specific CI configurations. This isn't inherently wrong, but be aware that most contributors won't run this test locally, so CI failures here might surprise them.
src/Traits/InitializesResources.php (1)
153-184: LGTM! Factory methods are consistent with existing patterns.The three new CSI resource factories follow the exact same pattern as existing resource factories in this trait. Consistent signatures, proper return types, clear docblocks. No issues.
src/Auth/TokenProvider.php (1)
47-52: LGTM! Fluent configuration method.The
setRefreshBuffermethod is clean, allows customization of the refresh buffer, and returnsstaticfor method chaining.src/Kinds/K8sService.php (1)
44-55: Clean enum integration.The
setType()andgetType()methods properly use the ServiceType enum with sensible defaults. The enum-based type system is a significant improvement over magic strings.src/Traits/Cluster/AuthenticatesCluster.php (1)
206-288: Token provider integration is well-designed.The new token provider architecture is cleanly integrated:
withTokenProvider()properly clears static tokens to avoid conflictsgetAuthToken()correctly prioritizes dynamic providers over static tokens- Helper methods (
withEksAuth,withOpenShiftAuth,withServiceAccountToken) provide convenient configurationThe pattern of passing
$thistoServiceAccountTokenProvider(line 276) for bootstrapping is clever and avoids tight coupling.tests/Auth/ExecCredentialIntegrationTest.php (4)
1-28: LGTM on setup and cluster availability check.The setup properly skips integration tests when no cluster is available, unless running in CI. The broad
\Exceptioncatch inisClusterAvailable()is appropriate here since we just want a boolean availability check.
85-119: Environment variable test looks good.The test uses
printenvto retrieve an environment variable containing the JSON credential. Same Unix-only caveat applies, but the approach is sound for testing env var propagation to the exec command.
70-83: YAML fixture is present and properly configured.The file
tests/yaml/kubeconfig-exec.yamlexists and contains the expected exec credential structure. The context 'exec-context' is properly defined, and the exec command will return the token 'exec-test-token' that the test asserts on line 77. No action needed.
30-68: The Windows concern doesn't apply here — CI only targets ubuntu-latest.The CI matrix (
runs-on: ubuntu-latest) is hardcoded and doesn't include Windows. Theechocommand will work fine in the test environment. No action needed.tests/Auth/ServiceAccountTokenIntegrationTest.php (4)
1-30: Slightly inconsistent exception handling compared to ExecCredentialIntegrationTest.
isClusterAvailable()here catchesKubernetesAPIException, while the same method inExecCredentialIntegrationTestcatches generic\Exception. Not a blocker—both work for their purpose—but worth noting for consistency.
32-66: Thorough token request test with proper cleanup.Good use of
try/finallyto ensure the service account is deleted. The expiration assertions (500-700s range for a 600s token) give reasonable buffer for test timing variance.
68-88: Audience test is minimal but sufficient.Tests that a token is retrieved with custom audiences. The actual audience claim verification would require JWT decoding, but asserting non-empty is reasonable for integration testing.
90-108: Convenience method test exercises the fluent API path.Tests
KubernetesCluster::fromUrl()->withServiceAccountToken()chain. Clean and covers the intended use case.tests/CSINodeTest.php (3)
1-37: LGTM on build test.Covers programmatic construction and validates all driver attributes (name, nodeID, topologyKeys, allocatable).
52-76: Driver method tests are comprehensive.Good coverage of both positive cases (existing driver) and negative cases (nonexistent driver returns null/empty). Clean assertions.
39-50: Fixture exists and is properly configured.The
tests/yaml/csinode.yamlfile is present with all expected data. Test assertions match the YAML structure.tests/Auth/ExecCredentialProviderTest.php (5)
1-25: Basic token extraction test is solid.Covers the happy path with minimal config.
27-58: Expiration and API version tests are thorough.Tests both v1 and v1beta1 API versions, and verifies expiration handling works correctly.
60-74: Environment variable propagation test is good.Tests that env vars defined in exec config are passed to the command.
76-119: Error handling tests cover the important cases.Missing token, invalid JSON, and failed command all throw
AuthenticationExceptionwith appropriate messages.
121-151: Cluster info test relies on reflection and incomplete verification.The test catches an exception and only verifies that
clusterInfowas stored—it doesn't verify thatKUBERNETES_EXEC_INFOwas actually set during the process execution. The current assertion only confirmssetClusterInfo()worked, not the actual env var propagation.That said, testing process environment variables is inherently tricky, and this approach is pragmatic.
tests/Auth/TokenProviderTest.php (4)
1-31: Mock providers are clean and focused.
MockTokenProviderallows controlling token and expiration.CountingTokenProvidertracks refresh calls. Both are minimal and purpose-built for the tests below.
33-68: Expiration tests cover key edge cases.
- No expiration → not expired
- Far future → not expired
- Past → expired
- Within 60s buffer → expired (important for refresh-before-expiry logic)
All correct.
70-98: Refresh triggering and custom buffer tests are solid.Verifies that:
- First
getToken()triggers refresh- Subsequent calls reuse cached token
- Custom buffer changes expiration threshold
Good coverage.
100-111: getExpiresAt test is straightforward.Verifies the expiration timestamp is accessible and formatted correctly.
src/Kinds/K8sPod.php (4)
11-12: Enum imports are appropriate.Clean integration with the new enum types.
212-231: Restart policy methods now use enum values—breaking change is documented.The PR summary notes
K8sPod::getRestartPolicy()now returnsRestartPolicyenum instead of string. Methods are consistent in their enum usage.
233-239: getRestartPolicy() handles missing spec gracefully.Falls back to
RestartPolicy::ALWAYS->valuewhich is the Kubernetes default for pods. Good.
427-459: getPodPhase() and phase-based methods are solid.
PodPhase::UNKNOWNexists in the enum, so the fallback ingetPodPhase()works as intended. The enum-backed approach forisRunning(),isSuccessful(), andisTerminal()is clean and removes conditional duplication. Implementation is correct.src/Auth/ServiceAccountTokenProvider.php (4)
1-49: Constructor validation and bootstrap token capture are solid.The design captures the bootstrap cluster's auth token at construction time to prevent infinite recursion when this provider is set on the same cluster. Validation for empty namespace/serviceAccount is clean.
51-63: Fluent configuration methods are straightforward.
withExpirationSeconds()andwithAudiences()follow the builder pattern consistently with the rest of the codebase.
65-120: refresh() implementation is robust.
- Uses
rawurlencode()for path segments (addresses prior feedback)- Properly separates
AuthenticationExceptionre-throw from generic exception wrapping- Fallback expiration calculation when
expirationTimestampis missingClean implementation.
122-135: makeBootstrapRequest() now has proper return type.Returns
ResponseInterfaceas expected. UsescallWithToken()to bypass the provider and avoid recursion.tests/VolumeAttributesClassTest.php (6)
1-30: Build test covers all accessors.Validates API version, name, labels, driver name, and parameters. Clean.
45-59: Version gate for Kubernetes 1.34+ is correct.VolumeAttributesClass is a recent API addition. The test correctly skips on older clusters.
61-132: CRUD tests are thorough.Creation, getAll, get by name, and update tests follow standard patterns. Labels update test verifies mutation works correctly.
134-150: Deletion test with polling is good.Uses a 60-second timeout with 2-second intervals to wait for deletion. Then expects exception on subsequent get—correct behavior.
152-170: Unused$typeparameter in watch callbacks is intentional.Static analysis flags
$typeas unused, but this is the standard callback signature for Kubernetes watch operations($type, $resource). Suppressing with@phpstan-ignore-next-lineor using$_typenaming convention would silence the warning, but it's not worth the noise.
32-43: YAML fixture file verification incomplete — sandbox environment prevented repository access.The test references
tests/yaml/volumeattributesclass.yamlvia__DIR__.'/yaml/volumeattributesclass.yaml', and if the test suite passes in CI/CD, the fixture exists. Manual verification of the file contents is required to confirm it provides the expected data structure matching the test assertions (api version, name, driver name, and parameters).src/KubernetesCluster.php (6)
7-7: LGTM on type modernization.The nullable property types and
staticreturn type for fluent chaining are solid PHP 8.2+ improvements. Clean and correct.Also applies to: 140-145, 158-163
171-187: Clean enum-based dispatch.The
tryFrom()fallback toGETpreserves backward compatibility for unknown string operations - exactly what was needed after the earlier review feedback. The match expression is readable and covers all cases cleanly.One reality check: operations like
WATCH,WATCH_LOGS, andATTACHexpect$payloadto be aClosure, but the signature acceptsstring|null|Closure. If someone passes a string where a Closure is expected, it'll blow up at runtime. That's existing behavior though, and fixing it would require splitting methods or more complex typing. Not a regression.
349-369: LGTM.Using
Operation::EXEC->httpMethod()is cleaner than hardcoding the HTTP method string. Themixedreturn type correctly reflects that this can return either an HTTP response or WebSocket result.
377-398: Consistent with execPath - looks good.
406-449: Consistent enum usage across all patch operations.All three patch methods (
applyPath,jsonPatchPath,jsonMergePatchPath) correctly use their respectiveOperationenum cases to derive HTTP methods. Clean modernization.
454-454: Type hint addition looks good.Adding explicit return type to
__callis a straightforward modernization with no behavioral change.composer.json (3)
37-38: Good call making AWS SDK optional.The
aws/aws-sdk-phpsuggestion is spot-on for the EKS token provider. Keeping it as a suggestion rather than a hard requirement means users who don't need EKS authentication won't bloat their dependencies. The ext-yaml formatting (period added) is trivial but consistent.
28-29: Both dependencies are actively used and necessary.composer/semveris imported insrc/Traits/Cluster/ChecksClusterVersion.phpfor semantic version comparison (Kubernetes API versioning).ext-jsonis extensively used across auth providers, cluster operations, and patches—17+ usages ofjson_encode()andjson_decode()throughout the codebase. These are legit additions, not dead weight.Likely an incorrect or invalid review comment.
34-34: Thesymfony/process7.3.4 constraint is correct—no issues.PHP 8.2+ matches Symfony 7.x requirements. The codebase uses
Process::fromShellCommandline(), which is a stable API. The constraint allows patch updates and is appropriate for this new dependency.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/VolumeSnapshotContentTest.php (1)
42-50: Delete this dead code—it was already flagged in the previous review.The
fromYamlFile()call returns a singleVolumeSnapshotContentobject, not an array. Thisis_array()block is unreachable dead code. The exact same code was removed fromVolumeSnapshotClassTestafter being flagged. Line 52 already handles the object directly.Remove lines 42-50.
🔎 Proposed fix
$vsc = $this->cluster->fromYamlFile(__DIR__.'/yaml/volumesnapshotcontent.yaml'); - // Handle case where CRD registration returns array - if (is_array($vsc)) { - foreach ($vsc as $instance) { - if ($instance instanceof VolumeSnapshotContent) { - $vsc = $instance; - break; - } - } - } - $this->assertInstanceOf(VolumeSnapshotContent::class, $vsc);
🧹 Nitpick comments (2)
AGENTS.md (2)
43-43: Align PHP version documentation with CI matrix.Line 43 mentions only "CI also exercises 8.5," but line 193 and the PR objectives clarify that CI tests PHP 8.2–8.5. Update line 43 to explicitly state all tested versions for consistency.
-**PHP:** `^8.2` (CI also exercises 8.5). +**PHP:** `^8.2` (CI tests 8.2–8.5).
54-56: Improve readability of consecutive test command descriptions.Three consecutive list items all begin with "Run," which creates repetitive phrasing. Minor refinement for clarity:
- **Run specific test file:** `vendor/bin/phpunit tests/PriorityClassTest.php`. - **Run specific test method:** `vendor/bin/phpunit tests/PriorityClassTest.php --filter test_priority_class_build`. - **Run with CI environment:** `CI=true vendor/bin/phpunit` (requires running Kubernetes cluster). + - **Target specific test file:** `vendor/bin/phpunit tests/PriorityClassTest.php`. + - **Target specific test method:** `vendor/bin/phpunit tests/PriorityClassTest.php --filter test_priority_class_build`. + - **Run with CI environment:** `CI=true vendor/bin/phpunit` (requires running Kubernetes cluster).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
AGENTS.mdsrc/Kinds/K8sCSIDriver.phptests/VolumeSnapshotContentTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Kinds/K8sCSIDriver.php
🧰 Additional context used
🧬 Code graph analysis (1)
tests/VolumeSnapshotContentTest.php (1)
tests/Kinds/VolumeSnapshotContent.php (18)
VolumeSnapshotContent(11-220)setDeletionPolicy(43-46)setDriver(63-66)setSnapshotHandle(126-129)setSourceVolumeMode(166-169)setVolumeSnapshotClassName(106-109)setVolumeSnapshotRef(83-89)getDeletionPolicy(53-56)getDriver(73-76)getSnapshotHandle(136-139)getSourceVolumeMode(176-179)getVolumeSnapshotClassName(116-119)getVolumeSnapshotRef(96-99)isReady(184-187)getRestoreSize(192-195)getSnapshotHandleFromStatus(200-203)getCreationTime(208-211)getError(216-219)
🪛 LanguageTool
AGENTS.md
[style] ~54-~54: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...les) to avoid cluster requirements. - Run specific test file: `vendor/bin/phpun...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~55-~55: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...punit tests/PriorityClassTest.php. - **Run specific test method:** vendor/bin/php...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~56-~56: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...-filter test_priority_class_build. - **Run with CI environment:** CI=true vendor/...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~202-~202: Consider a different adjective to strengthen your wording.
Context: ...i/) If anything is unclear or you need deeper context for a change, open a draft PR w...
(DEEP_PROFOUND)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (1)
tests/VolumeSnapshotContentTest.php (1)
172-188: Good—deletion polling was added.You addressed the flaky deletion issue from the previous review by adding the 60-second polling loop (lines 178-183). This matches the fix applied to
VolumeSnapshotClassTestand will prevent intermittent CI failures.
| * | ||
| * @throws \RenokiCo\PhpK8s\Exceptions\KubernetesAPIException | ||
| */ | ||
| public function callWithToken(string $token, string $method, string $path, string $payload = '', array $query = ['pretty' => 1], array $options = []) |
There was a problem hiding this comment.
The protected visibility was causing a problem. When ServiceAccountTokenProvider calls $this->bootstrapCluster->callWithToken(), since callWithToken() is protected, PHP can't access it directly (it's not in the inheritance hierarchy), so it triggers the __call() magic method in KubernetesCluster, which then proxies to K8s::callWithToken() (which doesn't exist properly), hence a weird return type.
In PHP, protected only works for inheritance, not for same-package access. Since ServiceAccountTokenProvider isn't a subclass of KubernetesCluster, it can't access protected methods.
The fix is to keep callWithToken() public.
cuppett
force-pushed
the
master
branch
2 times, most recently
from
086769d to
a462713
Compare
|
@rennokki Okay, I've done battle with the bots and I think this PR is ready for final review now and merge. This catches up functionally to my cuppett/php-k8s:main minus the vitepress documentation. Once merged, when you have a direction decided, just lmk and we can bang on a branch for it. If vitepress, I can likely port it all in very quickly. Also, I'm happy to click buttons in this repo if you want some help. Thanks again! |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/Auth/TokenProvider.php (1)
28-37: Timezone mismatch in expiration check is still present.Line 34 creates
new \DateTimewithout specifying a timezone, so it uses PHP's default (often the server's local timezone). If$this->expiresAtcomes from an API response in UTC (which is common), the comparison on line 36 will be incorrect.Real example: Server is in
America/New_York(UTC-5). Token expires at2025-12-29T15:00:00Z(UTC). At2025-12-29T09:30:00local time (14:30 UTC), the buffer check computes09:30 + 60s = 09:31 local, but compares against15:00 UTCparsed as15:00 local. The token appears valid when it should trigger refresh.🔎 Proposed fix
public function isExpired(): bool { if ($this->expiresAt === null) { return false; // No expiration known, assume valid } - $bufferTime = (new \DateTime)->modify("+{$this->refreshBuffer} seconds"); + $bufferTime = (new \DateTimeImmutable('now', new \DateTimeZone('UTC'))) + ->modify("+{$this->refreshBuffer} seconds"); + + // Normalize expiresAt to UTC for comparison + $expiresAtUtc = $this->expiresAt instanceof \DateTimeImmutable + ? $this->expiresAt->setTimezone(new \DateTimeZone('UTC')) + : \DateTimeImmutable::createFromInterface($this->expiresAt)->setTimezone(new \DateTimeZone('UTC')); - return $this->expiresAt <= $bufferTime; + return $expiresAtUtc <= $bufferTime; }src/Kinds/K8sCSINode.php (1)
47-47: Add missing return type declarations.Three methods still lack return type declarations despite having docblock types. This is inconsistent with other methods in the class (
getDrivers()andgetTopologyKeysForDriver()) that properly declare their return types.
- Line 47:
getDriverByName()needs: ?array- Line 73:
getNodeIdForDriver()needs: ?string- Line 95:
getAllocatableForDriver()needs: ?array🔎 Proposed fix
- public function getDriverByName(string $driverName) + public function getDriverByName(string $driverName): ?array- public function getNodeIdForDriver(string $driverName) + public function getNodeIdForDriver(string $driverName): ?string- public function getAllocatableForDriver(string $driverName) + public function getAllocatableForDriver(string $driverName): ?arrayAlso applies to: 73-73, 95-95
🧹 Nitpick comments (3)
AGENTS.md (2)
53-56: Reduce stylistic repetition in test command bullets.Lines 53–56 begin successive bullets with "Run", creating repetitive phrasing. Vary the structure for better readability and flow.
🔎 Proposed restructuring
- **Run tests (unit only):** `vendor/bin/phpunit --filter Test$` (or target specific files) to avoid cluster requirements. - **Run specific test file:** `vendor/bin/phpunit tests/PriorityClassTest.php`. - **Run specific test method:** `vendor/bin/phpunit tests/PriorityClassTest.php --filter test_priority_class_build`. - **Run with CI environment:** `CI=true vendor/bin/phpunit` (requires running Kubernetes cluster). + - **Unit tests only:** `vendor/bin/phpunit --filter Test$` (or target specific files) to avoid cluster requirements. + - **Specific test file:** `vendor/bin/phpunit tests/PriorityClassTest.php`. + - **Specific test method:** `vendor/bin/phpunit tests/PriorityClassTest.php --filter test_priority_class_build`. + - **With CI environment:** `CI=true vendor/bin/phpunit` (requires running Kubernetes cluster).
202-202: Improve closing sentence wording.The phrase "If anything is unclear or you need deeper context" is slightly awkward. "Deeper" doesn't strongly convey the intent—consider "additional context" or "more context" for clarity.
🔎 Proposed wording
- If anything is unclear or you need deeper context for a change, open a draft PR with your approach and questions. + If anything is unclear or you need additional context for a change, open a draft PR with your approach and questions.tests/VolumeAttributesClassTest.php (1)
154-158: Prefix unused callback parameters to silence static analysis.The
$typeparameter is required by the watch callback signature but unused in these tests. PHP convention is to prefix with underscore to document this is intentional.🔎 Proposed fix
- $watch = $this->cluster->volumeAttributesClass()->watchAll(function ($type, $vac) { + $watch = $this->cluster->volumeAttributesClass()->watchAll(function ($_type, $vac) { if ($vac->getName() === 'silver') { return true; } }, ['timeoutSeconds' => 10]);- $watch = $this->cluster->volumeAttributesClass()->watchByName('silver', function ($type, $vac) { + $watch = $this->cluster->volumeAttributesClass()->watchByName('silver', function ($_type, $vac) { return $vac->getName() === 'silver'; }, ['timeoutSeconds' => 10]);Also applies to: 165-167
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (51)
.github/workflows/ci.ymlAGENTS.mdPATCH_SUPPORT.mdcomposer.jsonexamples/patch_examples.phpsrc/Auth/EksTokenProvider.phpsrc/Auth/ExecCredentialProvider.phpsrc/Auth/OpenShiftOAuthProvider.phpsrc/Auth/ServiceAccountTokenProvider.phpsrc/Auth/TokenProvider.phpsrc/Contracts/TokenProviderInterface.phpsrc/Exceptions/AuthenticationException.phpsrc/Instances/ResourceMetric.phpsrc/Kinds/K8sCSIDriver.phpsrc/Kinds/K8sCSINode.phpsrc/Kinds/K8sPod.phpsrc/Kinds/K8sResource.phpsrc/Kinds/K8sScale.phpsrc/Kinds/K8sService.phpsrc/Kinds/K8sVerticalPodAutoscaler.phpsrc/Kinds/K8sVolumeAttributesClass.phpsrc/KubernetesCluster.phpsrc/Patches/JsonMergePatch.phpsrc/Patches/JsonPatch.phpsrc/Traits/Cluster/AuthenticatesCluster.phpsrc/Traits/Cluster/LoadsFromKubeConfig.phpsrc/Traits/Cluster/MakesHttpCalls.phpsrc/Traits/Cluster/MakesWebsocketCalls.phpsrc/Traits/InitializesResources.phpsrc/Traits/RunsClusterOperations.phptests/Auth/EksTokenProviderTest.phptests/Auth/ExecCredentialIntegrationTest.phptests/Auth/ExecCredentialProviderTest.phptests/Auth/OpenShiftOAuthProviderTest.phptests/Auth/ServiceAccountTokenIntegrationTest.phptests/Auth/TokenProviderTest.phptests/CSIDriverTest.phptests/CSINodeTest.phptests/Kinds/VolumeSnapshotClass.phptests/Kinds/VolumeSnapshotContent.phptests/KubeConfigTest.phptests/VolumeAttributesClassTest.phptests/VolumeSnapshotClassTest.phptests/VolumeSnapshotContentTest.phptests/cluster/exec-credential.jsontests/yaml/csidriver.yamltests/yaml/csinode.yamltests/yaml/kubeconfig-exec.yamltests/yaml/volumeattributesclass.yamltests/yaml/volumesnapshotclass.yamltests/yaml/volumesnapshotcontent.yaml
💤 Files with no reviewable changes (2)
- examples/patch_examples.php
- PATCH_SUPPORT.md
🚧 Files skipped from review as they are similar to previous changes (18)
- tests/yaml/volumesnapshotclass.yaml
- tests/yaml/csidriver.yaml
- src/Traits/Cluster/MakesWebsocketCalls.php
- src/Kinds/K8sVolumeAttributesClass.php
- src/Traits/InitializesResources.php
- tests/Auth/OpenShiftOAuthProviderTest.php
- tests/Auth/EksTokenProviderTest.php
- tests/cluster/exec-credential.json
- tests/Kinds/VolumeSnapshotClass.php
- src/Traits/Cluster/LoadsFromKubeConfig.php
- tests/yaml/csinode.yaml
- tests/Kinds/VolumeSnapshotContent.php
- tests/KubeConfigTest.php
- src/Patches/JsonPatch.php
- src/Kinds/K8sVerticalPodAutoscaler.php
- tests/Auth/TokenProviderTest.php
- tests/Auth/ServiceAccountTokenIntegrationTest.php
- tests/yaml/volumeattributesclass.yaml
🧰 Additional context used
🧬 Code graph analysis (19)
src/Kinds/K8sResource.php (2)
src/Patches/JsonMergePatch.php (1)
toJson(131-134)src/Patches/JsonPatch.php (1)
toJson(163-166)
src/Auth/ExecCredentialProvider.php (3)
src/Exceptions/AuthenticationException.php (1)
AuthenticationException(7-10)src/Auth/TokenProvider.php (2)
TokenProvider(8-55)refresh(54-54)src/Contracts/TokenProviderInterface.php (1)
refresh(22-22)
src/Auth/EksTokenProvider.php (3)
src/Exceptions/AuthenticationException.php (1)
AuthenticationException(7-10)src/Auth/TokenProvider.php (2)
TokenProvider(8-55)refresh(54-54)src/Contracts/TokenProviderInterface.php (1)
refresh(22-22)
src/Traits/Cluster/MakesHttpCalls.php (1)
src/Traits/Cluster/AuthenticatesCluster.php (1)
getAuthToken(223-230)
tests/CSINodeTest.php (6)
src/Kinds/K8sCSINode.php (7)
K8sCSINode(9-101)getDrivers(37-40)hasDriver(63-66)getDriverByName(47-58)getNodeIdForDriver(73-78)getTopologyKeysForDriver(83-88)getAllocatableForDriver(95-100)src/ResourcesList.php (1)
ResourcesList(7-10)src/Traits/InitializesResources.php (1)
csiNode(170-173)src/Traits/Resource/HasSpec.php (1)
setSpec(13-16)src/Traits/Resource/HasVersion.php (1)
getApiVersion(39-42)src/K8s.php (1)
fromYamlFile(57-66)
tests/Auth/ExecCredentialIntegrationTest.php (3)
tests/TestCase.php (1)
TestCase(12-199)src/Traits/Cluster/LoadsFromKubeConfig.php (2)
fromKubeConfigArray(104-109)fromKubeConfigYamlFile(94-97)src/Traits/Cluster/AuthenticatesCluster.php (1)
getAuthToken(223-230)
tests/VolumeSnapshotContentTest.php (1)
tests/Kinds/VolumeSnapshotContent.php (18)
VolumeSnapshotContent(11-220)setDeletionPolicy(43-46)setDriver(63-66)setSnapshotHandle(126-129)setSourceVolumeMode(166-169)setVolumeSnapshotClassName(106-109)setVolumeSnapshotRef(83-89)getDeletionPolicy(53-56)getDriver(73-76)getSnapshotHandle(136-139)getSourceVolumeMode(176-179)getVolumeSnapshotClassName(116-119)getVolumeSnapshotRef(96-99)isReady(184-187)getRestoreSize(192-195)getSnapshotHandleFromStatus(200-203)getCreationTime(208-211)getError(216-219)
src/Auth/TokenProvider.php (6)
src/Contracts/TokenProviderInterface.php (4)
getToken(12-12)isExpired(17-17)refresh(22-22)getExpiresAt(27-27)src/Auth/EksTokenProvider.php (1)
refresh(53-98)src/Auth/ExecCredentialProvider.php (1)
refresh(49-84)src/Auth/OpenShiftOAuthProvider.php (1)
refresh(44-117)src/Auth/ServiceAccountTokenProvider.php (1)
refresh(65-120)tests/Auth/TokenProviderTest.php (2)
refresh(14-18)refresh(25-30)
tests/VolumeSnapshotClassTest.php (1)
tests/Kinds/VolumeSnapshotClass.php (7)
VolumeSnapshotClass(8-88)setDriver(36-39)setDeletionPolicy(56-59)setParameters(76-79)getDriver(46-49)getDeletionPolicy(66-69)getParameters(84-87)
src/Auth/ServiceAccountTokenProvider.php (3)
src/Auth/TokenProvider.php (2)
TokenProvider(8-55)refresh(54-54)src/Traits/Cluster/AuthenticatesCluster.php (1)
getAuthToken(223-230)src/Traits/Cluster/MakesHttpCalls.php (1)
callWithToken(202-220)
src/Kinds/K8sCSIDriver.php (1)
src/Traits/Resource/HasSpec.php (2)
setSpec(13-16)getSpec(34-37)
src/Traits/Cluster/AuthenticatesCluster.php (4)
src/Auth/TokenProvider.php (1)
getToken(19-26)src/Contracts/TokenProviderInterface.php (1)
getToken(12-12)src/Auth/EksTokenProvider.php (1)
EksTokenProvider(7-129)src/Auth/OpenShiftOAuthProvider.php (2)
OpenShiftOAuthProvider(9-158)withoutSslVerification(37-42)
tests/VolumeAttributesClassTest.php (4)
src/Kinds/K8sVolumeAttributesClass.php (5)
K8sVolumeAttributesClass(8-66)setDriverName(36-39)setParameters(54-57)getDriverName(44-47)getParameters(62-65)src/Traits/Resource/HasVersion.php (1)
getApiVersion(39-42)src/Traits/Resource/HasAttributes.php (1)
isSynced(116-119)src/Traits/RunsClusterOperations.php (4)
createOrUpdate(131-140)delete(255-282)watch(407-423)watchAll(382-398)
src/Kinds/K8sService.php (6)
src/Kinds/K8sPod.php (1)
getClusterDns(48-53)src/Contracts/Dnsable.php (1)
getClusterDns(12-12)src/Traits/Resource/HasName.php (1)
getName(36-39)src/Traits/Resource/HasNamespace.php (1)
getNamespace(73-76)src/Traits/Resource/HasSpec.php (3)
setSpec(13-16)getSpec(34-37)addToSpec(24-27)src/Enums/ServiceType.php (1)
isExternallyAccessible(20-26)
src/Kinds/K8sCSINode.php (2)
src/Kinds/K8sResource.php (1)
K8sResource(23-257)src/Traits/Resource/HasSpec.php (1)
getSpec(34-37)
src/KubernetesCluster.php (4)
src/Traits/Cluster/MakesHttpCalls.php (1)
makeRequest(155-189)src/Enums/Operation.php (1)
httpMethod(29-38)src/K8s.php (1)
__call(122-129)src/Traits/Resource/HasAttributes.php (1)
__call(163-197)
src/Patches/JsonMergePatch.php (4)
src/Kinds/K8sResource.php (2)
toJson(141-144)toArray(116-133)src/Patches/JsonPatch.php (2)
toJson(163-166)toArray(153-156)src/Instances/ResourceMetric.php (1)
toArray(142-147)src/Instances/Instance.php (1)
toArray(23-26)
src/Kinds/K8sPod.php (3)
src/Traits/Resource/HasSpec.php (2)
setSpec(13-16)getSpec(34-37)src/Traits/Resource/HasStatusPhase.php (1)
getPhase(14-17)src/Enums/PodPhase.php (2)
isSuccessful(45-48)isTerminal(23-29)
src/Auth/OpenShiftOAuthProvider.php (3)
src/Exceptions/AuthenticationException.php (1)
AuthenticationException(7-10)src/Auth/TokenProvider.php (2)
TokenProvider(8-55)refresh(54-54)src/Contracts/TokenProviderInterface.php (1)
refresh(22-22)
🪛 LanguageTool
AGENTS.md
[style] ~54-~54: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...les) to avoid cluster requirements. - Run specific test file: `vendor/bin/phpun...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~55-~55: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...punit tests/PriorityClassTest.php. - **Run specific test method:** vendor/bin/php...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~56-~56: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...-filter test_priority_class_build. - **Run with CI environment:** CI=true vendor/...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~202-~202: Consider a different adjective to strengthen your wording.
Context: ...i/) If anything is unclear or you need deeper context for a change, open a draft PR w...
(DEEP_PROFOUND)
🪛 PHPMD (2.15.0)
tests/VolumeAttributesClassTest.php
154-154: Avoid unused parameters such as '$type'. (undefined)
(UnusedFormalParameter)
165-165: Avoid unused parameters such as '$type'. (undefined)
(UnusedFormalParameter)
tests/CSIDriverTest.php
149-149: Avoid unused parameters such as '$type'. (undefined)
(UnusedFormalParameter)
160-160: Avoid unused parameters such as '$type'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (42)
AGENTS.md (1)
1-202: Overall structure and content approved.The documentation is well-organized, comprehensive, and clearly written. Proper markdown headings, formatted links, accurate code examples, and practical guidance make this a solid developer reference. Once the CI version discrepancy is resolved, this file is ready.
src/Patches/JsonMergePatch.php (1)
128-133: Implementation is solid—throws instead of returning false.The use of
JSON_THROW_ON_ERRORwith the updated return type is the right call. The bitwise OR preserves user options, and the docblock correctly reflects the new throwing behavior. This eliminates the silent failure path that wasn't being checked by callers.tests/yaml/kubeconfig-exec.yaml (1)
1-20: LGTM - Clean test fixture.The kubeconfig structure is valid for testing exec-based authentication. Using
echowith a hardcoded JSON credential is appropriate for test fixtures, and the 2099 expiration ensures stable test behavior.src/Kinds/K8sScale.php (3)
6-6: LGTM - Enum import added for Operation migration.
96-106: LGTM - Clean enum migration in create().The migration from
KubernetesCluster::REPLACE_OPtoOperation::REPLACEis correct and aligns with the broader enum adoption across the codebase.
116-133: LGTM - Consistent enum usage in update().src/Exceptions/AuthenticationException.php (1)
1-10: LGTM - Empty exception class is acceptable.An empty exception class provides type-specific error handling, allowing authentication failures to be caught and handled distinctly from generic exceptions. This is a standard pattern.
src/Kinds/K8sResource.php (1)
141-144: LGTM - Accurate return type for json_encode behavior.The
string|falsereturn type correctly reflects thatjson_encode()returnsfalseon failure whenJSON_THROW_ON_ERRORis not used (which it isn't here). This aligns with the type-safety improvements across the PR.src/Instances/ResourceMetric.php (1)
142-147: LGTM - Explicit array return type.The method always returns an array via
array_merge(), so the: arrayreturn type is accurate. Consistent with the type-safety improvements in this PR.src/Contracts/TokenProviderInterface.php (1)
1-28: LGTM - Well-designed token provider contract.The interface provides a clean contract for token providers with appropriate method signatures. The use of
?DateTimeInterfacefor expiration accommodates tokens that don't expire, and the separation ofisExpired()andrefresh()allows for explicit control over token lifecycle.src/Kinds/K8sCSIDriver.php (7)
39-50: LGTM - Consistent fluent setter and getter pattern.
57-68: LGTM - Clean boolean flag implementation.
75-86: LGTM - Array getter with proper type hint.
93-104: LGTM - Boolean capacity tracking.
131-142: LGTM - Token requests array handling.
149-160: LGTM - Republish requirement flag.
167-178: LGTM - SELinux mount support flag.src/Kinds/K8sService.php (4)
34-40: LGTM - Namespace null-check now complete.The past review issue has been properly addressed. Both
$nameand$namespaceare now checked before constructing the DNS string, preventing the double-dot edge case when namespace is null.
45-56: LGTM - Clean ServiceType enum integration.The setter stores the enum's string value, and the getter reconstructs the enum using
ServiceType::from()with a sensible default. This is the correct pattern for enum persistence in specs.
61-64: LGTM - Good separation of concerns.Delegating the accessibility logic to the enum method keeps the behavior centralized and testable.
69-92: LGTM - Consistent fluent interface with static return types.src/Kinds/K8sPod.php (2)
212-239: Enum-based restart policy handling looks solid.The transition from string constants to
RestartPolicyenum values is clean. The setter accepts the enum and stores its string value, while the getter reconstructs the enum from the stored string with a sensible default (ALWAYS). This is a breaking change forgetRestartPolicy()return type, which is documented in UPGRADING.md per the PR objectives.
427-459: Pod phase enum integration is correct.The
getPodPhase()method handles null/empty phases by defaulting toUNKNOWN, which aligns with how Kubernetes reports pods that haven't been scheduled yet. TheisRunning(),isSuccessful(), andisTerminal()methods cleanly delegate to enum comparisons and thePodPhase::isTerminal()helper..github/workflows/ci.yml (2)
93-107: Cache upgrade and GitHub token addition are appropriate.The bump to
actions/cache@v5is sensible. AddingGITHUB_TOKENto the Minikube setup step prevents GitHub API rate limiting during the action's internal operations. Good housekeeping.
34-63: PHP 8.5 matrix entries are stable and supported—no CI risk here.PHP 8.5 reached general availability on November 20, 2025, with 8.5.1 as the current patch.
shivammathur/setup-phpfully supports PHP 8.5 with documented examples and changelog entries. You're not testing against an uncertain future; you're testing against a released, actively supported version with security fixes guaranteed through 2029. The CI won't break from PHP 8.5 being immature—it's already past that stage.Likely an incorrect or invalid review comment.
tests/yaml/volumesnapshotcontent.yaml (1)
1-14: VolumeSnapshotContent test fixture is correctly structured.The manifest covers required fields for a pre-provisioned VolumeSnapshotContent. The
hostpath.csi.k8s.iodriver matches the CSI hostpath addon enabled in CI. This will work for testing the K8sVolumeSnapshotContent resource class.src/Traits/RunsClusterOperations.php (2)
11-11: Operation enum import added for modernized cluster operations.Clean transition from string constants to the
Operationenum across allrunOperation()calls. This improves type safety and makes invalid operations a compile-time error rather than a runtime bug.
149-159: Operation enum usage across CRUD and specialized operations is consistent.Verified that
Operation::GET,CREATE,REPLACE,DELETE,APPLY,JSON_PATCH,JSON_MERGE_PATCH,WATCH,LOG,WATCH_LOGS,EXEC, andATTACHare all correctly mapped to their respective methods. The enum provides exhaustive coverage of Kubernetes API operations used by this library.src/Traits/Cluster/MakesHttpCalls.php (2)
84-87: Token resolution now usesgetAuthToken()for dynamic provider support.This change is the linchpin of the token provider architecture. Instead of directly accessing
$this->token, the code now callsgetAuthToken()which returns the provider's token if one is configured, otherwise falls back to the static token. This enables automatic token refresh through providers like EKS or OpenShift OAuth.
190-220:callWithToken()is correctly implemented with appropriate caveats.The method does what it says on the tin: temporarily swaps in a specific token, makes the call, then restores the original state. The docblock warning about async unsafety is accurate—PHP's standard request model is synchronous, but anyone using ReactPHP or similar should take note.
The public visibility is required since
ServiceAccountTokenProvidercalls this on an externalKubernetesClusterinstance, not from within the class hierarchy. This was already hashed out in previous review rounds.src/Auth/TokenProvider.php (1)
19-26: Lazy refresh pattern ingetToken()is correct.The method refreshes when the token is null or expired, then returns the (now-fresh) token. This ensures callers always get a valid token without needing to manage refresh timing themselves.
src/Auth/ExecCredentialProvider.php (3)
24-40: Constructor correctly parses Kubernetes exec credential config.The constructor handles the kubeconfig exec structure properly: extracting command, args, apiVersion, installHint, provideClusterInfo, and the name/value env array format. The fallback for
apiVersiontoclient.authentication.k8s.io/v1matches current Kubernetes defaults.
49-84:refresh()correctly implements Kubernetes exec credential protocol.The command execution properly escapes arguments, merges environment variables (intentionally including AWS credentials for EKS support), and sets
KUBERNETES_EXEC_INFOwhenprovideClusterInfois enabled. The error handling includes the optionalinstallHintfor user-friendly guidance.The full environment merge was discussed in prior reviews—it's intentional to support exec providers that rely on ambient credentials (AWS, GCP, etc.).
86-113: JSON parsing with proper error handling—fixed as requested.The
JSON_THROW_ON_ERRORflag with try/catch cleanly separates JSON parse failures from missing token fields. Including a truncated output snippet in the error message aids debugging without flooding logs with potentially large credential outputs.Setting
expiresAttonullwhen no expiration timestamp is provided correctly signals toisExpired()that the token should be treated as non-expiring.composer.json (2)
29-29: Thecomposer/semverpackage is actively used in the codebase. It's imported and used insrc/Traits/Cluster/ChecksClusterVersion.phpfor bothComposer\Semver\ComparatorandComposer\Semver\VersionParser. The addition is justified.
28-34: Thesymfony/process^7.3.4 constraint is valid.Version 7.3.4 exists on Packagist and is available for installation. No issue here.
tests/VolumeSnapshotContentTest.php (1)
1-179: LGTM - all previous issues resolved.Both previously flagged issues have been properly addressed:
- Dead array handling removed (line 42 now correctly asserts the single object)
- Deletion polling added (lines 168-173 prevent race conditions)
The test suite comprehensively covers build, YAML loading, status methods, and full API lifecycle. Clean implementation.
tests/VolumeSnapshotClassTest.php (1)
1-150: LGTM - previous issues resolved.Both flagged issues have been fixed:
- Dead array handling removed with clarifying comment (line 34)
- Deletion polling added (lines 139-144)
Test coverage is comprehensive and follows established patterns.
src/Auth/ServiceAccountTokenProvider.php (1)
1-135: LGTM - all previous issues addressed.All flagged concerns have been properly resolved:
- Return type added to
makeBootstrapRequest()(line 126)- Bootstrap token validated in constructor (lines 32-36)
- Namespace and service account validated (lines 38-44)
- Path segments URL-encoded (lines 67-71)
- Exception handling preserves
AuthenticationExceptionmessages (lines 109-111)The recursion-prevention architecture using
callWithToken()with the captured bootstrap token is solid. Input validation is thorough.tests/VolumeAttributesClassTest.php (1)
11-150: Test structure is solid.The test suite properly covers:
- Fluent builder API
- YAML loading
- Version-gated API interactions (1.34+ requirement at line 48)
- Full CRUD lifecycle with deletion polling (lines 140-145)
- Watch operations
Implementation follows established patterns from other resource tests.
src/KubernetesCluster.php (2)
171-188: LGTM - backward-compatible enum conversion.The
tryFrom() ?? Operation::GETfallback at lines 174-176 correctly preserves the previous behavior where unknown operation strings defaulted to GET. This prevents breaking existing callers while enabling type-safe enum-based dispatch.The match expression is clean and explicit.
140-140: Comprehensive type hints added.All properties and methods now have explicit types:
- Properties:
?stringwith null defaults- Path methods:
: mixedreturn types__call:: mixedreturn typeThis aligns with the PR's modernization goals. The
mixedreturn type is appropriate since these methods can return various types (ResourcesList, K8sResource, strings, null, etc.).Also applies to: 145-145, 158-158, 195-195, 280-280, 352-352, 381-381, 406-406, 423-423, 440-440, 454-454
1 participant
This PR brings significant modernization and feature additions to php-k8s:
📦 Changes Overview
1. PHP 8.2+ Modernization: Enums, Match Expressions, and Type Hints (#45)
Operation,PodPhase,RestartPolicy,Protocol,ServiceType,ConditionStatus,AccessMode,PullPolicyOperationenum and match expressionsBreaking Changes:
KubernetesClusteroperation constants removed (useOperationenum)K8sPod::getRestartPolicy()now returnsRestartPolicyenum2. Advanced Authentication Support
3. CSI Storage Resources (#52)
K8sCSIDriver,K8sCSINode,K8sVolumeAttributesClass(core API)VolumeSnapshotClass,VolumeSnapshotContent4. Cleanup and Documentation
PATCH_SUPPORT.mdandexamples/patch_examples.phpAGENTS.mddocumenting authentication architecture🧪 Testing
🔄 Compatibility
📝 Notes
(This PR catches up all functionality to cuppett/php-k8s:main. Once we have this in, we can pull from the updated/expanded vitepress documentation into whatever/wherever format is desired. Thanks!
Ready for review! This PR consolidates multiple feature additions for easier batch review.