Conversation
WalkthroughThis PR modifies the Dockerfile to change the base image from Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/wandb/spec/charts/repo_test.go (2)
276-304: Consider explicitly validating logged messages in debug mode.
Although capturing logs in a Ginkgo environment can be tricky, verifying exact log statements would further strengthen test coverage.
474-487: Recording transport is straightforward, but consider concurrency safety.
If multiple goroutines could invoke this transport concurrently in future tests, you may need a lock around request recording. Otherwise, it’s a neat helper class for single-threaded cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Dockerfile(1 hunks)Tiltfile(2 hunks)pkg/wandb/spec/channel/deployer/deployer.go(1 hunks)pkg/wandb/spec/channel/deployer/deployer_test.go(4 hunks)pkg/wandb/spec/charts/repo.go(1 hunks)pkg/wandb/spec/charts/repo_test.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Test
- GitHub Check: build-and-scan
🔇 Additional comments (18)
Dockerfile (1)
35-36: LGTM: Added CA certificates from ubi-minimal imageGood addition! Copying the CA certificates from the ubi-minimal image ensures that TLS verification can work properly in the container. This is essential for secure communication with external services.
Tiltfile (2)
19-20: LGTM: Added global watch settings with appropriate ignoresThe addition of global watch settings with a 2-second debounce and specific ignore patterns will improve development performance by preventing unnecessary rebuilds.
31-31: LGTM: Consistent CA certificate handling in TiltfileThis change correctly mirrors the Dockerfile update to ensure CA certificates are available in the development environment as well.
pkg/wandb/spec/charts/repo.go (1)
126-135:Details
❓ Verification inconclusive
LGTM: Improved TLS verification logic
This change correctly modifies the TLS verification logic to only disable verification for HTTP URLs, while maintaining proper verification for HTTPS URLs. The error handling for URL parsing is also appropriately implemented.
However, I noticed that on line 164, there's still a hard-coded
getter.WithInsecureSkipVerifyTLS(true)option that might conflict with this improved logic:getter.WithInsecureSkipVerifyTLS(true),
🏁 Script executed:
#!/bin/bash # Search for any calls to the DownloadTo method to check how the InsecureSkipVerifyTLS setting is used rg -A 3 "DownloadTo" --type goLength of output: 292
Action Required: Align TLS Verification Settings
The updated TLS verification logic now disables TLS verification only for HTTP URLs and handles URL parsing errors appropriately. However, the hard-coded option (
getter.WithInsecureSkipVerifyTLS(true)) on line 164 may still force TLS verification to be skipped regardless of the URL scheme. Please verify whether this remaining option is intentional. If its behavior conflicts with the improved logic, consider updating it—possibly by conditionally applying it only when the URL scheme is HTTP—to ensure consistent TLS verification across the code.pkg/wandb/spec/charts/repo_test.go (5)
4-5: Imports are well-aligned with new TLS and JSON testing needs.
No concerns here; these additions look appropriate and necessary for the updated context usage, JSON encoding, and TLS testing.Also applies to: 12-13
195-195: Good use of context.
Switching fromniltocontext.TODO()is a positive step for improved clarity and potential timeout/cancellation management.
223-243: Comprehensive TLS verification table test.
These cases cover HTTP vs. HTTPS (including uppercase variations) thoroughly. Nice job ensuring that TLS errors don’t overshadow standard HTTP error responses.
245-274: Logical check for InsecureSkipTLSverify in HTTP vs. HTTPS.
Verifying the direct URL parsing logic is clear and ensures correctness for case-insensitive handling of schemes.
305-409: Robust self-signed certificate tests.
These scenarios capture the correct failure modes and demonstrate that turning on “InsecureSkipTLSverify” or adjusting the transport can handle self-signed certs. The coverage is commendable.pkg/wandb/spec/channel/deployer/deployer_test.go (9)
4-11: New imports for context, TLS, JSON, and spec charts are appropriate.
They align with the new test scenarios for TLS validation, JSON marshalling, and context usage.Also applies to: 19-19
74-99: Solid testing for server error handling.
Retrying on 5xx responses and ultimately failing is a key scenario to confirm robust error handling.
101-119: Good coverage for unreachable server scenario.
Verifying that we exhaust retries and surface a clear error strengthens reliability.
121-151: Timeout handling is well-exercised.
Simulating the server’s delayed response ensures the client’s timeout logic is correct, enhancing stability under real-world conditions.
153-174: Invalid JSON test ensures graceful error handling.
Catching parsing errors instead of silently failing is essential. This covers a common edge case.
176-248: TLS certificate validation tests are comprehensive.
Distinguishing between trusted and untrusted certs at the transport layer is crucial for secure communications. Nicely done.
250-292: Container CA certificate checks are helpful.
Verifying that the environment has correct CA directories and can handle secure sites is a practical addition—particularly in container-based deployments.
315-353: Automated Dockerfile CA cert test is valuable.
Confirming that CA paths exist ensures minimal surprises in container environments. Nice approach for catching misconfigurations early.
356-414: Manual test for real endpoint connectivity is clearly marked.
Skipping by default is sensible, and providing a toggle for explicit runs is best practice for environment-dependent tests.
### [1.18.5](v1.18.4...v1.18.5) (2025-02-26) ### Bug Fixes * TLS ([#67](#67)) ([0d3013c](0d3013c))
|
This PR is included in version 1.18.5 🎉 |
## 1.0.0 (2025-04-11) ### Features * Add active-state cm ([wandb#2](https://github.com/sqaisar/wandb-operator/issues/2)) ([5a6c4c3](5a6c4c3)) * Add caching for deployer release requests ([1185b40](1185b40)) * Add events recording ([388d37b](388d37b)) * Add helm support ([077765c](077765c)) * Add option to set reconcileFrequency ([484c014](484c014)) * Add support for helm repo releases ([dfef752](dfef752)) * Add support for release from a git repository ([8a6b073](8a6b073)) * Adding owner ref and wait and timeout to uninstall ([wandb#51](https://github.com/sqaisar/wandb-operator/issues/51)) ([f21fd6d](f21fd6d)) * Allow the operator to support installation without cluster level permissions ([wandb#16](https://github.com/sqaisar/wandb-operator/issues/16)) ([6f29a3e](6f29a3e)) * Make wandb operator available on OperatorHub ([wandb#32](https://github.com/sqaisar/wandb-operator/issues/32)) ([1a59dab](1a59dab)) * **operator:** Add airgapped support ([wandb#12](https://github.com/sqaisar/wandb-operator/issues/12)) ([bfd3796](bfd3796)) * Prevent Logging of Sensitive info in Plain Text ([wandb#31](https://github.com/sqaisar/wandb-operator/issues/31)) ([5530cb3](5530cb3)) * Prevent Logging of Sensitive info in Plain Text ([wandb#35](https://github.com/sqaisar/wandb-operator/issues/35)) ([9a752fd](9a752fd)) * Release Version Pinning Init ([wandb#28](https://github.com/sqaisar/wandb-operator/issues/28)) ([dfe8bda](dfe8bda)) * Replace base image with RHEL UBI ([wandb#44](https://github.com/sqaisar/wandb-operator/issues/44)) ([12497d2](12497d2)) * Support for deploymenting via jobs ([da801ea](da801ea)) * Updated license.go file to include the feature for licenseSecret as well ([wandb#54](https://github.com/sqaisar/wandb-operator/issues/54)) ([bb55caa](bb55caa)) * Use container based deployments only ([3e6b222](3e6b222)) * use secrets instead of configmaps ([049797f](049797f)) ### Bug Fixes * add applied config to download bundle ([bef77c2](bef77c2)) * Add console namespace and service name to config properties ([0b9efef](0b9efef)) * Add debugging for installing release ([893ebd9](893ebd9)) * add gh token for ci ([72d456f](72d456f)) * add license log ([wandb#11](https://github.com/sqaisar/wandb-operator/issues/11)) ([e129fab](e129fab)) * Add operator namespace env ([846731a](846731a)) * add operator properties to config ([b5f48f0](b5f48f0)) * add pnpm, node and git to docker image ([176b6f0](176b6f0)) * Add Tilt configs for local development ([wandb#53](https://github.com/sqaisar/wandb-operator/issues/53)) ([5ef82b5](5ef82b5)) * added changelog commits ([61b5f5d](61b5f5d)) * Assign metadata instead of merging it ([908c839](908c839)) * Basic Auth Fix ([wandb#56](https://github.com/sqaisar/wandb-operator/issues/56)) ([414b2cf](414b2cf)) * Bump controller tools version to latest ([wandb#13](https://github.com/sqaisar/wandb-operator/issues/13)) ([c52dbb6](c52dbb6)) * Bump deps ([wandb#36](https://github.com/sqaisar/wandb-operator/issues/36)) ([eefb59c](eefb59c)) * Bump deps ([wandb#70](https://github.com/sqaisar/wandb-operator/issues/70)) ([11ba9f8](11ba9f8)) * Channel spec not getting applied correctly ([6e763a8](6e763a8)) * Charts download ([57355ce](57355ce)) * Clean up docker image ([ef7c629](ef7c629)) * clean up env for image push ([7213ed2](7213ed2)) * Correct merge order ([cd49cef](cd49cef)) * correctly check if chart is installed based on status ([384d330](384d330)) * Create release rc files ([f7f4622](f7f4622)) * Debug logging errors ([wandb#26](https://github.com/sqaisar/wandb-operator/issues/26)) ([a641621](a641621)) * Debug logging the cache ([wandb#21](https://github.com/sqaisar/wandb-operator/issues/21)) ([26e8fd5](26e8fd5)) * Debugging logic ([wandb#22](https://github.com/sqaisar/wandb-operator/issues/22)) ([2c019b8](2c019b8)) * Default to dev mode ([d961f77](d961f77)) * docker build ([d160a9c](d160a9c)) * docker image push ([e08b3da](e08b3da)) * Git release pulls correctly ([d47aebd](d47aebd)) * init controller ([0f0a9e9](0f0a9e9)) * install go version ([6664b4b](6664b4b)) * Install kubectl in docker image ([e5df9de](e5df9de)) * Jobs work? ([9972d26](9972d26)) * kubectl not working in docker image ([ffc694e](ffc694e)) * Local_resource and kubectl apply could risk deploying to the wrong context ([wandb#66](https://github.com/sqaisar/wandb-operator/issues/66)) ([a90a270](a90a270)) * lock pnpm version ([c2608f7](c2608f7)) * Log the diff of specs ([wandb#23](https://github.com/sqaisar/wandb-operator/issues/23)) ([c0ea0d8](c0ea0d8)) * Look for secret in namespace of wandb CR ([wandb#78](https://github.com/sqaisar/wandb-operator/issues/78)) ([e374c9a](e374c9a)) * Mask sensitive values in log ([wandb#14](https://github.com/sqaisar/wandb-operator/issues/14)) ([514336d](514336d)) * merge func ([94aa0d0](94aa0d0)) * Output json format logs ([90af7b6](90af7b6)) * Pass namespace into chart ([e8e0b8f](e8e0b8f)) * pass spec namespace and name ([79d77f2](79d77f2)) * Preserve unknown fields ([565a25f](565a25f)) * properly get license ([6ff6533](6ff6533)) * Properly merge chart specs together ([37c41bc](37c41bc)) * Properly parse chart from deployer ([5eabdfe](5eabdfe)) * Properly set namespace for deployments ([53f51a9](53f51a9)) * Properly update complete status ([86a5196](86a5196)) * push images to dockerhub ([d4cdd27](d4cdd27)) * refactor spec ([87be86b](87be86b)) * Refactor specs ([7c6da34](7c6da34)) * Release needs ginkgo ([wandb#65](https://github.com/sqaisar/wandb-operator/issues/65)) ([c51df78](c51df78)) * remove console ([fba45ee](fba45ee)) * remove debugging logs ([d4da31f](d4da31f)) * remove submodule ([bdb408a](bdb408a)) * Remove ui building step ([08ee985](08ee985)) * Rename config -> values and release -> chart ([519cd1b](519cd1b)) * Rename config spec cfs ([672100a](672100a)) * rename configs ([8727281](8727281)) * rename docker variables ([274e20c](274e20c)) * rename versioning step name ([77bf4ed](77bf4ed)) * reorder backup ([ab66486](ab66486)) * revert to v2 for semver ([535a721](535a721)) * Save active spec metadata ([47bd862](47bd862)) * Secret reading metadata ([6dab7ed](6dab7ed)) * secrets stored with correct values ([f6d61e9](f6d61e9)) * Serve console with gin ([c9e04aa](c9e04aa)) * set namespace when running kubectl apply ([1d6f00c](1d6f00c)) * Setting cached release namespace incorrectly ([e585555](e585555)) * Simplify docker image ([1cf55e4](1cf55e4)) * Support Openshift permissions schema for the helm cache ([wandb#17](https://github.com/sqaisar/wandb-operator/issues/17)) ([b498f79](b498f79)) * TLS ([wandb#67](https://github.com/sqaisar/wandb-operator/issues/67)) ([0d3013c](0d3013c)) * Tmp directory permissions ([b0820f5](b0820f5)) * Update to fix some CVEs ([wandb#55](https://github.com/sqaisar/wandb-operator/issues/55)) ([9a34cbe](9a34cbe)) * upgrade semantic to v3 ([594c463](594c463)) * Use cdk8s image for apply container ([189bc08](189bc08)) * Use deployer release channels ([480b380](480b380)) * Using validate for job spec ([5c7ff66](5c7ff66)) * x-kubernetes-preserve-unknown-fields ([bedac52](bedac52))
3 participants
Summary by CodeRabbit
New Features
Tests
DeployerClientto cover various failure scenarios and TLS certificate validation.