-
Notifications
You must be signed in to change notification settings - Fork 8
Fix crd generation to put files in the right directory #171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughController-gen targets updated to restrict paths and adjust CRD output directories. The Lease CRD schema adds beginTime and endTime, makes duration optional with clarified semantics, and tweaks a status description. Test setup now loads CRDs from the new Helm CRD directory. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deploy/helm/jumpstarter/crds/jumpstarter.dev_leases.yaml (1)
141-144: Consider clarifying the status description.Line 144's description reads: "If the lease has been acquired an exporter name is assigned and then it can be used, it will be empty while still pending"
This could be reworded for clarity, perhaps:
"Set when the lease acquires an exporter. Empty while pending exporter assignment."Apply this diff to improve clarity:
description: |- - If the lease has been acquired an exporter name is assigned - and then it can be used, it will be empty while still pending + Set when the lease acquires an exporter and becomes active. + Empty while the lease is pending exporter assignment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Makefile(1 hunks)deploy/helm/jumpstarter/crds/jumpstarter.dev_leases.yaml(3 hunks)internal/controller/suite_test.go(1 hunks)
⏰ 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). (6)
- GitHub Check: lint-go
- GitHub Check: deploy-kind
- GitHub Check: tests
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: e2e-tests-28d6b1cc3b49ab9ae176918ab9709a2e2522c97e
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
🔇 Additional comments (5)
internal/controller/suite_test.go (1)
66-66: LGTM! CRD path correctly updated.The test environment now loads CRDs from the new directory structure, aligning with the Makefile changes that generate CRDs to
deploy/helm/jumpstarter/crds/.Makefile (2)
52-54: LGTM! Improved controller-gen targeting.Restricting paths to
./api/...and./internal/...is better practice than./...as it:
- Avoids accidentally processing vendored dependencies
- Excludes test code and fixtures
- Makes manifest generation more deterministic
The CRD output directory change to
deploy/helm/jumpstarter/crds/aligns with the test configuration update.
58-58: LGTM! Consistent path narrowing for code generation.Using narrower paths here matches the manifests target and follows the same best practices.
deploy/helm/jumpstarter/crds/jumpstarter.dev_leases.yaml (2)
134-136: LGTM! Correctly updated required fields.Removing
durationfrom the required list is consistent with the new schema where duration can be calculated frombeginTimeandendTime.
52-83: Validation covers all Lease time field combinations. TheReconcileLeaseTimeFieldsfunction inlease_helpers.go
- computes
DurationfromBeginTime+EndTime,- computes
BeginTimefromEndTime+Duration,- rejects missing
Duration(including EndTime-only or none provided),- and errors on any conflict between all three fields.
No further controller changes required.
Summary by CodeRabbit
Enhancements
Documentation
Chores