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

Folder: Replace folderId with folderUid #58393

Merged
merged 24 commits into from Nov 17, 2022
Merged

Folder: Replace folderId with folderUid #58393

merged 24 commits into from Nov 17, 2022

Conversation

lpskdl
Copy link
Contributor

@lpskdl lpskdl commented Nov 8, 2022

What is this feature?

Replace folderId with folderUid in the following areas:

  • Folder picker in unified alerting
  • Returning to view mode after editing a panel
  • Folder picker in dashboard general settings
  • Saving dashboard
  • pre-selecting folder in save dashboard form modal
  • uid in routes when opening a dashboard
  • saving dashboard json in dashboard settings
  • dashboard/folder name validation
  • moving dashboard to another folder
  • dashboard action buttons
  • dashboard creations on an empty folder
  • showing library panel
  • saving a library panel

Why do we need this feature?

  • Prework needed for nested folder feature. Although the backend will still support backward compatibility of using folderId it would be easier to start on cleaner state when we start the actual feature as they will be mostly working with folderUids

Blocked by issue #57988. This is needed to avoid supporting folderId and folderUid in ImportDashboardForm component.

Which issue(s) does this PR fix?:

Fixes #57874

Special notes for your reviewer:

  • Import dashboard isn't working at the moment with folderUid it will be solve when api/dashboards/import API isn't working with folderUid #57988 is addressed
  • DashList and AlertList are still using folderId as the api to get those list aren't supporting folderUid at the moment. Separate issue will be created for that and be assigned to the backend.

@lpskdl lpskdl added area/dashboard area/alerting Grafana Alerting area/frontend no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Nov 8, 2022
@lpskdl lpskdl added this to the 9.3.0 milestone Nov 8, 2022
@lpskdl lpskdl requested review from a team November 8, 2022 09:27
@lpskdl lpskdl requested review from joshhunt and a team as code owners November 8, 2022 09:27
@lpskdl lpskdl self-assigned this Nov 8, 2022
@grafanabot
Copy link
Contributor

Copy link
Member

@soniaAguilarPeiron soniaAguilarPeiron left a comment

Choose a reason for hiding this comment

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

Looks great from alerting side! 🎉

@lpskdl lpskdl requested a review from a team as a code owner November 14, 2022 09:13
@lpskdl lpskdl requested review from sakjur, zserge and idafurjes and removed request for a team November 14, 2022 09:13
@grafanabot
Copy link
Contributor

@leandro-deveikis leandro-deveikis modified the milestones: 9.3.0, 9.3.0-beta1 Nov 14, 2022
@kaydelaney
Copy link
Contributor

Would it be possible for this to get merged soon? :)

@lpskdl
Copy link
Contributor Author

lpskdl commented Nov 15, 2022

Would it be possible for this to get merged soon? :)

Hi @kaydelaney, I would like to ask if you can have a review on the FolderPicker changes please :) If all good I will merge it

@grafanabot
Copy link
Contributor

This pull request was removed from the 9.3.0-beta1 milestone because 9.3.0-beta1 is currently being released.

@grafanabot grafanabot removed this from the 9.3.0-beta1 milestone Nov 15, 2022
@grafanabot
Copy link
Contributor

@grafanabot
Copy link
Contributor

@lpskdl lpskdl added this to the 9.4.0 milestone Nov 16, 2022
Copy link
Contributor

@JoaoSilvaGrafana JoaoSilvaGrafana left a comment

Choose a reason for hiding this comment

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

Nice one! 👏

Comment on lines 206 to 209
if (
(fullDash.meta.folderUid === undefined && fullDash.meta.folderId === null && toFolder.uid === '') ||
fullDash.meta.folderUid === toFolder.uid
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of curiosity, which scenarios does folderUid === undefined && folderId === null? is it some sort of legacy thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good eyes @JoaoSilvaGrafana it suppose to be folderUid. About the scenario, It seems part of a legacy thing, I'm not entirely sure which scenario it catches as it always go to the second condition(fullDash.meta.folderUid === toFolder.uid).

Probably there might be a case where folderUid is undefined or null and if this happen the same as the destination folder is '' which is general then we assume that they are in the same general folder location hence sending { alreadyInFolder: true };

@grafanabot
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/alerting Grafana Alerting area/backend area/dashboard area/frontend enterprise-failed no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace the use of folderId with folderUids in library panels, unified alerting and dashboard
8 participants