Skip to content
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

Support a trunk based development like Hyperion does with feature flags #534

Closed
stan-dot opened this issue Jul 2, 2024 · 12 comments
Closed
Labels
enhancement New feature or request help wanted Extra attention is needed python Pull requests that update Python code spike Issue to investigate scope/feasilibilty/technology etc. of how to do something

Comments

@stan-dot
Copy link
Collaborator

stan-dot commented Jul 2, 2024

Would allow us a gradual rollout of features using env flags with a fine-grained control between beamlines, with all of them being potentially on the latest version of the main flow.

@DominicOram , @dperl-dls please send some pointers

main area this would see use is in the endpoints
example implementation

from dotenv import load_dotenv
import os

load_dotenv()  # Load environment variables from .env file

class Settings(BaseSettings):
    secret_route_enabled: bool = os.getenv('SECRET_ROUTE_ENABLED', 'False').lower() in ('true', '1', 't')

settings = Settings()

settings = Settings()
def feature_flag_enabled(feature_name: str):
    def decorator(func):
        @wraps(func)
        async def wrapper(*args, **kwargs):
            if feature_name == 'secret_route' and not settings.secret_route_enabled:
                raise HTTPException(status_code=404, detail="Not Found")
            return await func(*args, **kwargs)
        return wrapper
    return decorator


@app.get("/secret", response_class=HTMLResponse)
@feature_flag_enabled('for-i22-November')
async def secret_route(request: Request):
@stan-dot stan-dot added enhancement New feature or request help wanted Extra attention is needed python Pull requests that update Python code spike Issue to investigate scope/feasilibilty/technology etc. of how to do something labels Jul 2, 2024
@DominicOram
Copy link
Contributor

Could you give some examples of what features you would like to enable/disable with flags like this? The example you have here is a particular endpoint. In hyperion we do not use feature flags for whether an endpoint is visible or not, as that doesn't change the behaviour of the system. Instead we would use a flag in GDA for whether a particular endpoint is called. I'm not sure I see where a feature flag like this fits into blueAPI?

@stan-dot
Copy link
Collaborator Author

stan-dot commented Jul 9, 2024

I see the feature flags as a catch-all for maintaining trust but at the same time experimenting with new things. With feature flags we can selectively deploy various features. (The assumption here is that we keep features behind endpoints and they add to the features set of the system rather than change it. The latter would change only in major versions I guess?).

Whether the feature flag is in the client or the server is an architecture question where I'd say that permissive client and restrictive server is the more natural way. Furthermore if blueapi has many clients (cli,jython and a GUI in the future) maintaining consistent feature flags across the 3 of the would be much more difficult compared to just in the server

@DominicOram
Copy link
Contributor

But if it's feature flags for endpoints we would need logic in the clients anyway to say whether they try and call the endpoints? Otherwise the clients will fail if they expect endpoints to exist that aren't there due to feature flags? It seems simpler that if features are new endpoints we just have expose those endpoints regardless then have flags in the clients about whether we want to use them or not?

@stan-dot
Copy link
Collaborator Author

stan-dot commented Jul 9, 2024

we could configure the endpoint to return a meaningful message: 'your server instance does support this feature' and the clients would render that gracefully without failing

@stan-dot
Copy link
Collaborator Author

@callumforrester , @DiamondJoseph request for comments

@callumforrester
Copy link
Collaborator

Makes sense to me, @DominicOram's view carries most weight as he's been using feature flags for a while now

@DominicOram
Copy link
Contributor

Can we please clarify what this issue is for exactly? The title is very vague. Is it actually "Add the ability to enable endpoints based on a feature flag from an .env file"? If you want to add this to blueAPI I'm not against it but I can't see any point where we would use it in MX. Feature flags on the client side make a lot more sense to me.

@stan-dot
Copy link
Collaborator Author

title is not exhaustive, there is also description. the main idea is for the DX and trunk based development, as opposed to long-lived side branches.

whether the feature flag is in an .env file is just an implementation detail.

if we have an API accessible to any client then the feature flags in just one client don't protect from unsupported requests to the server, if it is not protected.

from a DX perspective, I'd like to push a half-done endpoint on Friday with an env flag making it visible or not. Then I can just checkout main on Tuesday machine day and switch the flag for my testing purposes. Instead of doing that all in a branch.

I am aware that this is opinionated.

@DominicOram
Copy link
Contributor

the main idea is for the DX and trunk based development, as opposed to long-lived side branches.

Completely agree with this, but that's a process point in your team not an actionable thing that you can write a blueAPI issue about. The issue should be focused on what code changes are required to enable this and the title should reflect that. IMO I require no blueAPI changes to enable this but if you feel you do then that's fine, I'm just asking you to specify what you need to do that.

@stan-dot
Copy link
Collaborator Author

stan-dot commented Aug 1, 2024

I moved the 'example implementation' for flag-guarded routes from a comment into the description

@callumforrester
Copy link
Collaborator

I'm also satisfied that no code changes are needed for now, but am also interested in @keithralphs and @DiamondJoseph's opinions

@stan-dot
Copy link
Collaborator Author

stan-dot commented Aug 8, 2024

not needed anymore

@stan-dot stan-dot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed python Pull requests that update Python code spike Issue to investigate scope/feasilibilty/technology etc. of how to do something
Projects
None yet
Development

No branches or pull requests

3 participants