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

PublicDashboards: Add setting to disable the feature #78894

Merged
merged 23 commits into from Dec 19, 2023

Conversation

AgnesToulet
Copy link
Contributor

What is this feature?
This PR adds a setting in the configuration files to disable the public dashboards feature (enabled by default):

[public_dashboards]
enabled = true

Why do we need this feature?
We want to remove the publicDashboards feature toggle as the feature is now in GA but we should keep the possibility to disable the feature as it has security implications. We want to add this setting before remove the feature toggle to give time to users who wants to disable the feature to update their configuration file.

Who is this feature for?
Users who want do disable the public dashboards features.

Which issue(s) does this PR fix?:

Fixes #78050

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@AgnesToulet AgnesToulet marked this pull request as ready for review December 11, 2023 09:34
@AgnesToulet AgnesToulet requested review from juanicabanas and removed request for a team December 11, 2023 09:34
Copy link
Collaborator

@imatwawana imatwawana left a comment

Choose a reason for hiding this comment

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

I made some small grammar edits and shortened the same phrase in several places. Otherwise, LGTM!

docs/sources/dashboards/assess-dashboard-usage/index.md Outdated Show resolved Hide resolved
docs/sources/setup-grafana/configure-grafana/_index.md Outdated Show resolved Hide resolved
pkg/services/featuremgmt/registry.go Outdated Show resolved Hide resolved
pkg/services/featuremgmt/toggles_gen.go Outdated Show resolved Hide resolved
pkg/services/publicdashboards/api/api.go Outdated Show resolved Hide resolved
Copy link
Contributor

@IevaVasiljeva IevaVasiljeva left a comment

Choose a reason for hiding this comment

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

LGTM from the IAM perspective and in general.

I wonder if it could be made more explicit in the docs that the feature toggle still needs to be enabled to enable public dashboards. But otherwise all looks good 👌

@AgnesToulet
Copy link
Contributor Author

@IevaVasiljeva As the feature toggle is enabled by default, users don't really have to know about this. They just need to not explicitly set the feature toggle to false for the feature to work.

Co-authored-by: Isabel <76437239+imatwawana@users.noreply.github.com>
@IevaVasiljeva
Copy link
Contributor

@IevaVasiljeva As the feature toggle is enabled by default, users don't really have to know about this. They just need to not explicitly set the feature toggle to false for the feature to work.

Thanks for clarifying, I didn't realise that the feature toggle was enabled by default. All looks good to me then :)

Copy link
Contributor

@evictorero evictorero left a comment

Choose a reason for hiding this comment

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

LGTM! Just left a comment to think about it.
The cutoff for Grafana 11 is March 29, we should create and issue and put something in our calendar to make the changes to remove the flag.

conf/defaults.ini Show resolved Hide resolved
pkg/services/publicdashboards/api/api.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@imatwawana imatwawana left a comment

Choose a reason for hiding this comment

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

LGTM!

@evictorero
Copy link
Contributor

We should check the enterprise code, I think we are using this flag somewhere (besides the email sharing one).

Copy link
Collaborator

@chri2547 chri2547 left a comment

Choose a reason for hiding this comment

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

Just two small tweaks. Thanks for the contribution!

public/app/features/admin/UserListPage.tsx Outdated Show resolved Hide resolved
@@ -61,7 +61,7 @@ export class ShareModal extends SceneObjectBase<ShareModalState> implements Moda
}
}

if (Boolean(config.featureToggles['publicDashboards'])) {
if (Boolean(config.featureToggles['publicDashboards']) && config.publicDashboardsEnabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we could reuse the new function here

@AgnesToulet
Copy link
Contributor Author

We should check the enterprise code, I think we are using this flag somewhere (besides the email sharing one).

Yes the Enterprise PR is ready to be reviewed as well (just forgot to put our squad as reviewers)

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a SharePublicDashboardUtils.ts file. I think we should add this function there.
I think it should also be renamed to utils.ts because of the rest of the project

@AgnesToulet AgnesToulet merged commit fdaf6e3 into main Dec 19, 2023
19 checks passed
@AgnesToulet AgnesToulet deleted the agnestoulet/remove-publicdashboards-feature-flag branch December 19, 2023 10:43
@summerwollin summerwollin modified the milestones: 10.3.x, 10.3.0 Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Task: Remove publicDashboards FF
7 participants