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

Connections: Simplify connections nav #66813

Merged
merged 3 commits into from May 2, 2023
Merged

Conversation

torkelo
Copy link
Member

@torkelo torkelo commented Apr 19, 2023

Slack thread: https://raintank-corp.slack.com/archives/C034LMXLXQU/p1681835191084819

As discussed over slack this simplifies and flattens the nav hierarchy in Connections

  • Make Connect data a normal page and not a section with no children
  • Remove the nested section "Your connections" as it has so few children (only 1 in OSS)
  • Rename Connect data to "Add new connection" to make it clear that this is where you go to create new connections.

Could need help from the connections devs (@leventebalogh ?) to complete this PR as it might need changes in plugins (and in plugin nav config).

I also did not change any urls in this PR (they still include /your-connections) I think it might be good to change those as well. And the connect data page that was renamed is connect-data in URL as I assume that is referenced by a plugin config to override that URL.

image

@leventebalogh
Copy link
Contributor

leventebalogh commented Apr 19, 2023

Thanks for opening this @torkelo! 👍

I think technically this change is not complicated, we have to pay attention to change things in other places accordingly as you said. I also agree that we should update the URLs (and the nav-ids), too, to keep the code in a consistent state.

Changes required in other places

  • Update the page paths in the grafana-easystart-app here (it is needed to be able to register them correctly under the Connections section using the Grafana config)
  • Update the hosted-grafana configuration here (It is the Grafana INI configuration that is setting which section the standalone pages from the plugin should be registered to. In case we update the URLs in core - which I believe we should - then these need to updated, too.)

Another (important) question:
As the cloud instances would break if we publish the updates of core and the plugins separately, how do we make sure that they get deployed "at the same time"?

Also summoning @mikkancso and @jarben.

@mikkancso
Copy link
Contributor

As the cloud instances would break if we publish the updates of core and the plugins separately, how do we make sure that they get deployed "at the same time"?

I think that's the critical question. WDYT about adding that config to the default Grafana config? That way probably we wouldn't have to synchronize the config update in Hosted-Grafana-API with the release. I'm gonna ask the Grafana SaaS team about this.

@mikkancso
Copy link
Contributor

@mikkancso
Copy link
Contributor

Once https://github.com/grafana/hosted-grafana/issues/3859 is done, we should be fine and we can move forward with this PR.

@jarben
Copy link
Contributor

jarben commented Apr 21, 2023

Once https://github.com/grafana/hosted-grafana/issues/3859 is done, we should be fine and we can move forward with this PR.

@mikkancso isn't this already supported here? Or could we just use if inst.ActualVersion.Compare(semver.MustParse("10.0.0-0")) >= 0 if we want to keep one feature flag only?

Also this change may affect this issue in hg-cloud-home-admin. @yduartep I'd suggest to wait with implementing this issue after G10. Looks like the current redirect in the plugin will handle this change without an issue @mikkancso ?

We also need to update the PDC plugin to reflect this, cc @fabienne-m

@yduartep
Copy link
Contributor

yduartep commented Apr 21, 2023

Once grafana/hosted-grafana#3859 is done, we should be fine and we can move forward with this PR.

@mikkancso isn't this already supported here? Or could we just use if inst.ActualVersion.Compare(semver.MustParse("10.0.0-0")) >= 0 if we want to keep one feature flag only?

Also this change may affect this issue in hg-cloud-home-admin. @yduartep I'd suggest to wait with implementing this issue after G10. Looks like the current redirect in the plugin will handle this change without an issue @mikkancso ?

We also need to update the PDC plugin to reflect this, cc @fabienne-m

I have not worked on this issue yet because I have seen that there is an automatic redirect and then I wanted to wait for a complete rollout of the nav. In any case, I will wait after Grafana 10 to see what happens. Thanks for the update @jarben .

@mikkancso
Copy link
Contributor

@mikkancso isn't this already supported here? Or could we just use if inst.ActualVersion.Compare(semver.MustParse("10.0.0-0")) >= 0 if we want to keep one feature flag only?

Thanks, I haven't realized that. I think we should go with your latter suggestion.

Also this change may affect this https://github.com/grafana/hg-cloud-home-admin/issues/291. @yduartep I'd suggest to wait with implementing this issue after G10. Looks like the current redirect in the plugin will handle this change without an issue @mikkancso ?

Yeah, I think we can prepare the plugin to redirect to the correct url based on the Grafana version. I'll look into this now in depth.

@mikkancso
Copy link
Contributor

I updated this PR with the renaming of the URLs, component names and navIDs.
I also opened PRs in cloud-onboarding and hosted-grafana repos (see above ☝️ ). They are all draft PRs at the moment, but the whole thing together seems to be working.

@torkelo torkelo marked this pull request as ready for review April 29, 2023 17:53
@torkelo torkelo requested review from a team as code owners April 29, 2023 17:53
@torkelo torkelo requested review from sakjur, papagian and mildwonkey and removed request for a team April 29, 2023 17:53
@torkelo
Copy link
Member Author

torkelo commented Apr 29, 2023

@mikkancso @leventebalogh so when can we merge this?

@mikkancso
Copy link
Contributor

@mikkancso @leventebalogh so when can we merge this?

I think we can merge this as soon as checks pass on this PR

@grafanabot
Copy link
Contributor

Hello @torkelo!
Backport pull requests need to be either:

  • Pull requests which address bugs,
  • Urgent fixes which need product approval, in order to get merged,
  • Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If the pull request modifies CI behaviour, please add the type/ci label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

@torkelo torkelo added the product-approved Pull requests that are approved by product/managers and are allowed to be backported label May 2, 2023
@torkelo torkelo merged commit 9614dc2 into main May 2, 2023
22 of 23 checks passed
@torkelo torkelo deleted the simplify-connections-nav branch May 2, 2023 08:52
grafanabot pushed a commit that referenced this pull request May 2, 2023
* Connections: Simplify connections nav

* rename Connections pages everywhere

---------

Co-authored-by: Miklós Tolnai <miklos.tolnai@grafana.com>
(cherry picked from commit 9614dc2)
@zerok zerok modified the milestones: 10.0.0, 10.1.x May 3, 2023
torkelo added a commit that referenced this pull request May 3, 2023
Connections: Simplify connections nav (#66813)

* Connections: Simplify connections nav

* rename Connections pages everywhere

---------

Co-authored-by: Miklós Tolnai <miklos.tolnai@grafana.com>
(cherry picked from commit 9614dc2)

Co-authored-by: Torkel Ödegaard <torkel@grafana.com>
@ricky-undeadcoders ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/backend area/frontend backport v10.0.x product-approved Pull requests that are approved by product/managers and are allowed to be backported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants