feat(codereadiness): add hook for version-based flag validation - #904
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds the ChangesCodereadiness hook and package setup
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
marcin11858
changed the title
1cbe435 to
f73bc0c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hooks/codereadiness/pkg/codereadiness.go`:
- Around line 58-61: The readiness check in codereadiness should treat a missing
FlagMetadata as a validation failure when strict validation is enabled, instead
of returning nil. Update the logic around FlagMetadata handling in
codereadiness.go so that the same validation path used for a missing
minCodeVersion key also runs when metadata is nil, while preserving the current
non-strict behavior.
- Around line 27-30: WithComparator currently allows a nil comparator to replace
the default from New(), which can make Hook.After panic on first use. Update
WithComparator to either ignore a nil argument and preserve the existing
comparator, or fail fast by rejecting nil before assigning h.comparator, using
the WithComparator and Hook.After flow as the main reference points.
In `@hooks/codereadiness/README.md`:
- Around line 69-77: The custom comparator example is missing the required fmt
import, so the snippet is not copy-pasteable as shown. Update the README example
around customComparator to either include fmt in the example’s imports or
replace fmt.Errorf with an error construction that does not require fmt,
ensuring the sample remains complete and directly usable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3bde35c9-65a7-49c4-801d-2cf06c04e29d
⛔ Files ignored due to path filters (1)
hooks/codereadiness/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
.release-please-manifest.jsonhooks/codereadiness/LICENSEhooks/codereadiness/README.mdhooks/codereadiness/go.modhooks/codereadiness/pkg/codereadiness.gohooks/codereadiness/pkg/codereadiness_test.gorelease-please-config.json
f73bc0c to
e3c782f
Compare
oxddr
left a comment
oxddr
left a comment
There was a problem hiding this comment.
This looks good overall, I left couple of comments regarding style and clarity of the code.
f7c9bb7 to
1e06c44
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hooks/codereadiness/pkg/codereadiness_test.go (1)
98-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a helper to reduce duplicated
of.InterfaceEvaluationDetailsboilerplate.Nearly every test builds the same nested
of.InterfaceEvaluationDetails{ EvaluationDetails: of.EvaluationDetails{ ResolutionDetail: of.ResolutionDetail{ FlagMetadata: ... } } }structure. A small helper (e.g.newDetails(metadata of.FlagMetadata) of.InterfaceEvaluationDetails) alongsidenewTestHook/newTestHookContextwould remove this repetition across ~12 test functions and make each test body focus on the metadata under test.♻️ Proposed helper
func newDetails(metadata of.FlagMetadata) of.InterfaceEvaluationDetails { return of.InterfaceEvaluationDetails{ EvaluationDetails: of.EvaluationDetails{ ResolutionDetail: of.ResolutionDetail{ FlagMetadata: metadata, }, }, } }Also applies to: 116-122, 135-141, 153-159, 172-180, 192-199, 213-220, 233-240, 253-260, 270-277, 291-299, 312-319, 339-346, 363-370, 402-410
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hooks/codereadiness/pkg/codereadiness_test.go` around lines 98 - 104, The test file repeats the same nested of.InterfaceEvaluationDetails construction in many places, so add a small helper near newTestHook/newTestHookContext (for example, newDetails(metadata of.FlagMetadata) of.InterfaceEvaluationDetails) that builds the shared EvaluationDetails/ResolutionDetail wrapper with the provided metadata. Then replace the duplicated inline boilerplate in the affected test functions with calls to that helper so each test only specifies the metadata it cares about.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@hooks/codereadiness/pkg/codereadiness_test.go`:
- Around line 98-104: The test file repeats the same nested
of.InterfaceEvaluationDetails construction in many places, so add a small helper
near newTestHook/newTestHookContext (for example, newDetails(metadata
of.FlagMetadata) of.InterfaceEvaluationDetails) that builds the shared
EvaluationDetails/ResolutionDetail wrapper with the provided metadata. Then
replace the duplicated inline boilerplate in the affected test functions with
calls to that helper so each test only specifies the metadata it cares about.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e596a30-f18f-414e-975e-3ce9b315172b
⛔ Files ignored due to path filters (1)
hooks/codereadiness/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
.release-please-manifest.jsonhooks/codereadiness/LICENSEhooks/codereadiness/README.mdhooks/codereadiness/go.modhooks/codereadiness/pkg/codereadiness.gohooks/codereadiness/pkg/codereadiness_test.gorelease-please-config.json
✅ Files skipped from review due to trivial changes (3)
- .release-please-manifest.json
- hooks/codereadiness/LICENSE
- hooks/codereadiness/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- release-please-config.json
- hooks/codereadiness/go.mod
- hooks/codereadiness/pkg/codereadiness.go
1e1e758 to
a64e669
Compare
erka
left a comment
erka
left a comment
There was a problem hiding this comment.
Could we make this a flat package without pkg subpackage? I think we don't need it.
c5f69f0 to
ee015a3
Compare
2012c46 to
fabd180
Compare
Done, removed |
e63c606 to
043f662
Compare
erka
left a comment
erka
left a comment
There was a problem hiding this comment.
It looks great!
Just one small nit: we've started adding GoDoc examples, and it would be great to include one here as well. Something along the lines of the example in the OpenFeature Go SDK:
https://github.com/open-feature/go-sdk/blob/main/openfeature/isolated/isolated_example_test.go
|
Code looks good ... one non-code question:
|
Thank you for your question. I found two possible use cases:
|
7bb18e4 to
8a71844
Compare
Done. Added some examples in the |
31eac32 to
06fe6e8
Compare
The usefulness of this check is much more apparent with trunk-based development, i.e. single, stable main branch, from which releases are cut on regular basis (e.g. daily or weekly). In this setup, it's common, that a partially completed feature ends up in the new release. This hook is to provide code-level safety - as pointed by you. Internally, we pair it with some other safety mechanisms. This is our last line of defence. |
Introduce the codereadiness hook to control feature flag evaluation by comparing the application's current version with a required minimum version specified in the flag's metadata. If the comparator returns an error the hook also returns an error to trigger fallback to the default flag value. Signed-off-by: Marcin Wlazły <marcinwlazly@google.com>
4bdc096 to
477b371
Compare
|
I'm ready to merge this, but please consider my question here first! 🙏 |
5 participants
feat(codereadiness): Introduce the
codereadinesshook to control feature flag evaluation by comparing the application's current version with a required minimum version specified in the flag's metadata. If the comparator returns an error the hook also returns an error to trigger fallback to the default flag value.This PR
Introduces the
codereadinesshook underhooks/codereadinessto control feature flag evaluation based on the application's running code version.How it works
minCodeVersionkey).WithComparator), custom metadata keys (viaWithMetadataMinVerKey) and optional strict validation (viaWithValidationRequired).Notes
codereadiness_test.go.README.mdwith setup and configuration examples.Follow-up Tasks
None.
How to test
Run the package unit tests: