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

feat: add toggle for hiding public url #6641

Merged
merged 1 commit into from
Feb 11, 2025
Merged

Conversation

mindspank
Copy link
Contributor

Toggle to hide publicUrl.

Checklist:

  • Covered by tests
  • Ran it and it works as intended
  • Reviewed the diff before requesting a review
  • Checked for unhandled edge cases
  • Linked the issues it closes
  • Checked if the docs need to be updated
  • Intend to cherry-pick into the release branch
  • I'm proud of this work!

@mindspank
Copy link
Contributor Author

Closes #6313

@mindspank mindspank merged commit 4196884 into main Feb 11, 2025
9 checks passed
@mindspank mindspank deleted the feat/public-url-toggle branch February 11, 2025 15:19
@royendo
Copy link
Contributor

royendo commented Feb 12, 2025

Does it make more sense to have hidepublicUrl default to false and have a customer require to set this true to remove publicurl or have publicurl and default this to true and have them set it to false when they want to remove it?

@mindspank
Copy link
Contributor Author

Does it make more sense to have hidepublicUrl default to false and have a customer require to set this true to remove publicurl or have publicurl and default this to true and have them set it to false when they want to remove it?

Right now it defaults to false. So if you set the flag it would remove the option to create public links

#rill.yaml

features:
  - hidePublicUrl

@ericpgreen2
Copy link
Contributor

Does it make more sense to have hidepublicUrl default to false and have a customer require to set this true to remove publicurl or have publicurl and default this to true and have them set it to false when they want to remove it?

Roy, good catch, totally agree. For example, we have a feature flag exports that defaults to true, we don't have a flag named hideExports that defaults to false.

Alex, the feature flag block accepts a dictionary like:

features:
  clickhouseModeling: true

@mindspank
Copy link
Contributor Author

@ericpgreen2 but maybe we should be descriptive in the toggle name for permanent toggles.

Both from a code perspective but also for the user.

features:
  - hideExport

Reads cleaner then

features:
  - export: false

@ericpgreen2
Copy link
Contributor

I can see it either way. But the longer the list of toggled features, then I think this...

features:
  canvasDashboards: true
  cloudDataViewer: true
  exports: false
  publicUrls: false

... is more clear than...

features:
  - canvasDashboards
  - cloudDataViewer
  - hideExports
  - hidePublicUrls

It's confusing to see a mix of positives & negatives. And, semantically, I'd argue "exports" is the feature, not "hiding exports".

@mindspank
Copy link
Contributor Author

Not agreeing on this one, pure feature names in toggles should ideally be avoided as by default released features should be on and not off.

This would also read cleaner when we extend feature toggles to urls and Auth payloads as you would do feature=hideExport for example

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.

3 participants