feat(codereadiness): prepare Code Readiness hook specification#405
feat(codereadiness): prepare Code Readiness hook specification#405marcin11858 wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis change adds a new "Code Readiness Hook" specification section to Appendix A, defining requirements for a hook that blocks feature flag activation on binaries below a minimum code version specified in flag metadata, including lifecycle behavior, initialization, and configurable comparison strategy. ChangesDocumentation Addition
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
3b523f0 to
76b6397
Compare
Signed-off-by: Marcin Wlazły <marcinwlazly@google.com>
76b6397 to
3ee0571
Compare
|
|
|
I can see you are implementing this in a few repos. I'm quite curious about it. Copy pasting my question from here:
I'm not fundamentally opposed to this, but I want to make sure I understand the use case before we add this to the spec - we don't have many hooks in the spec. If we do want to spec it, we also may want to consider including it in the base SDK - but that's a separate question. |
Thank you for your question. I found two possible use cases: working on a complex task: The developer introduced reading feature flag in one of the PR but there is more PRs in a queue which are waiting for a review. When all PRs will be merge the code is ready and new feature can be enable safety. |
|
Also pasting my reply from open-feature/go-sdk-contrib#904 (comment) 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 partially completed feature ends up in the new release. This hook is to provide code-level mechanism to prevent enabling those unfinished features. Internally we pair this code-level checks with some other safety mechanisms. This is our last line of defence. |
|
@oxddr @marcin11858 OK thanks for the explanations. Personally, I see the use case - not 100% sure about the name - but the idea of a code-level blocker for a certain feature (distinct from implementing a version check in targeting rules) seems valuable to me; it's a distinct boundary enforced by the code author, rather than whomever might have access to the flag management plane. 👍 However, I question whether or not it really needs to be in the spec, as it seems somewhat niche. I am perfectly OK with having it in all the contribs, and will help to have all instances merged, but we have lots of items in the contribs that are not in the spec. The contribs basically represent extensions from the spec - additional things that we don't specify or include in the SDK. If you really believe this is important enough to be in the spec, I think we should move the implementations to the SDKs themselves. Things such as the What is your feeling about this? Do you think this needs to be in the spec and in the SDKs? Or are you satisfied to maintain it in the contribs as more of an "extension"? |
I agree. It's an interesting idea that's likely valuable in some situations. However, it doesn't feel like something that needs to be in the spec. If it turns out to be a highly used feature, we could consider adding it to the spec and possibly even natively supporting it in the SDKs. |
|
Link open-feature/java-sdk-contrib#1819 as well to this. |
|
We don't have strong preferences to have this in the spec. I believe it was suggested to put it here during one of code reviews. For us it's fine to have the implementation in languages we're interested about and treat it like extension, similarly to lots of other hooks. I think it may be worthy to put this description somewhere (maybe alongside one of the implementations), so whenever someone wants to contribute the implementation in other languages, we can make them follow the same spec for sake of consistency. |
|
The OpenTelemetry hook is in go-sdk-contrib, and it has its own Appendix D, so it really depends on how important this is. If this hook is implemented across several SDKs, it would be better to have a common reference point for all of them. I think this PR is a great starting point for establishing a shared vision for this hook, even if it isn't merged and remains open for a while. Just my two cents. |
We have some telemetry-related helpers and constants in various SDKs, which are related to appendix D, for example: https://github.com/open-feature/js-sdk/blob/main/packages/shared/src/telemetry/attributes.ts - these are intended to be used for hooks etc, so there is at least some representation of appendix D in SDKs directly (we don't want to add the OTel API as a dep in the SDK itself).
Ya I agree - and to be clear, I'm not opposed to adding it if there's enough interest. Let's continue with the implementations for now, and we can revisit later if we actually want to specify this. |
This PR
Describes specification of the Code Readiness Hook. The Hook will prevent enabling a feature flag in running binaries that are not ready
Follow-up Tasks
None