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

Use GetInstance to load feature flags instead of rill.yaml #4800

Merged
merged 1 commit into from
May 2, 2024

Conversation

AdityaHegde
Copy link
Collaborator

closes #4763

We cannot use repo APIs in cloud to get rill.yaml. Feature flags was moved to GetInstance. This PR updates to make use of GetInstance.

@AdityaHegde AdityaHegde added the blocker A release blocker issue that should be resolved before a new release label May 2, 2024
@briangregoryholmes
Copy link
Contributor

briangregoryholmes commented May 2, 2024

If this PR is not a release blocker, I think it would be a good time to solidify our precedence for different feature flag methods going forward.

I'm of the opinion that we should remove setting feature flags via localStorage or the URL. If others say we need to maintain that, then we'll need to change the feature flag implementation so that certain flags cannot be "overridden". For instance, you might set exports to false in a rill.yaml and a user should not be able to force this to true by appending something to the URL. But that may not be true of every feature flag. Currently, there's this idea of system vs user, but I think we should change this scope idea to rill vs user and then add an additional method/property to determine whether something can be set via the URL or localStorage.

@ericpgreen2

@AdityaHegde
Copy link
Collaborator Author

AdityaHegde commented May 2, 2024

Good point. Ideally we should have a check in the backend as well. Because it is trivial to force change it on client side and still enable export.

@AdityaHegde AdityaHegde removed the blocker A release blocker issue that should be resolved before a new release label May 2, 2024
@AdityaHegde
Copy link
Collaborator Author

@begelundmuller Since the issue was with an incomplete release, do you think this is a blocker still?

@begelundmuller
Copy link
Contributor

@begelundmuller Since the issue was with an incomplete release, do you think this is a blocker still?

I don't think it should be a release blocker

@ericpgreen2
Copy link
Contributor

If this PR is not a release blocker, I think it would be a good time to solidify our precedence for different feature flag methods going forward.

I'm of the opinion that we should remove setting feature flags via localStorage or the URL.

@ericpgreen2

I'm game for this. Both setting flags via local storage and via URL preceded our ability to set flags in rill.yaml. It'll be simpler to have just one way of operating. Both ourselves and customers can handle rill.yaml.

Copy link
Contributor

@ericpgreen2 ericpgreen2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, since removing URL & local storage feature flag support can always be done in a separate PR.

@ericpgreen2 ericpgreen2 merged commit 610cb4b into main May 2, 2024
6 checks passed
@ericpgreen2 ericpgreen2 deleted the remove-rill-yaml-calls branch May 2, 2024 23:55
k-anshul pushed a commit that referenced this pull request May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix: Do not request rill.yaml in rill cloud
4 participants