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

Service accounts: Remove Add API keys buttons and remove one state of migrating for API keys tab #63411

Merged

Conversation

eleijonmarck
Copy link
Contributor

@eleijonmarck eleijonmarck commented Feb 18, 2023

What is this feature?
This is part of our deprecation strategy for API keys. This does the following:

  • refactor: removes any add API keys elements from the frontend
  • refactor: removes the migrationstatus state in frontend and backend
  • refactor our copy on the API keys page to reflect the deprecation with clear wording
  • displays migration if any API keys are present
  • displays hiding the API keys tab if no API keys are present

Why do we need this feature?
This feature is an improvement of our communication and functionality of deprecating API keys from Grafana. We were showing a bit too much about migration status which gave impression of migrating would be a big issue. This improves not only the performance by reducing a number of calls to the backend but also creates a better experience for the user flow (tested and approved by UX)

*Which issue(s) does this PR fix?**:
#58091

Fixes #

Special notes for your reviewer:

@eleijonmarck eleijonmarck added this to the 9.4.1 milestone Feb 20, 2023
@eleijonmarck eleijonmarck marked this pull request as ready for review February 20, 2023 10:00
@eleijonmarck eleijonmarck requested a review from a team as a code owner February 20, 2023 10:00
@eleijonmarck eleijonmarck requested review from linoman and alexanderzobnin and removed request for a team February 20, 2023 10:00
@eleijonmarck eleijonmarck changed the title Service accounts: add hide apikeys tab on start Service accounts: Add hide apikeys tab on start Feb 20, 2023
Copy link
Contributor

@alexanderzobnin alexanderzobnin left a comment

Choose a reason for hiding this comment

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

Some issues needs to be resolved. Also, we check and hide API keys tab for specific org. But then new org can be created and that org will have API keys tab. That's kinda strange behavior. Maybe we should have a global flag?

pkg/services/serviceaccounts/manager/service.go Outdated Show resolved Hide resolved
pkg/services/serviceaccounts/manager/service.go Outdated Show resolved Hide resolved
pkg/services/serviceaccounts/manager/service.go Outdated Show resolved Hide resolved
pkg/services/serviceaccounts/manager/service.go Outdated Show resolved Hide resolved
@eleijonmarck eleijonmarck requested a review from a team as a code owner February 20, 2023 15:47
@eleijonmarck eleijonmarck requested review from zserge, mildwonkey, yangkb09 and alexanderzobnin and removed request for a team February 20, 2023 15:47
@eleijonmarck eleijonmarck requested a review from a team February 20, 2023 16:12
Copy link
Contributor

@alexanderzobnin alexanderzobnin left a comment

Choose a reason for hiding this comment

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

I have a few comments to consider.

pkg/services/serviceaccounts/manager/service.go Outdated Show resolved Hide resolved
pkg/services/serviceaccounts/api/api_test.go Outdated Show resolved Hide resolved
pkg/services/serviceaccounts/manager/service.go Outdated Show resolved Hide resolved
pkg/services/serviceaccounts/manager/service.go Outdated Show resolved Hide resolved
pkg/services/serviceaccounts/serviceaccounttest/fake.go Outdated Show resolved Hide resolved
@vtorosyan
Copy link
Contributor

Could we please hold on with the merge until we align on the release milestone and understand how this should be communicated. As this is a breaking change, I am hesitant about letting it out with 9.5.


// hideApiKeysTabIfNoAPIKeysPresent is used to remove the apikeys tab from the admin tabs
// if there are no apikeys, this makes it so that we only issue service account tokens instead
func hideApiKeysTabIfNoAPIKeysPresent(l *log.ConcreteLogger, store *sqlstore.SQLStore, serviceAccountsStore *database.ServiceAccountsStoreImpl) {
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: This should be part of the store

Copy link
Contributor

@kalleep kalleep Feb 21, 2023

Choose a reason for hiding this comment

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

Also I think the correct place would be in the api key package

@eleijonmarck eleijonmarck changed the title Service accounts: Add hide apikeys tab on start Service accounts: Refactor API keys tab Feb 23, 2023
@eleijonmarck eleijonmarck requested review from joshhunt and a team as code owners February 23, 2023 16:06
@eleijonmarck eleijonmarck requested review from ashharrison90 and removed request for a team February 23, 2023 16:06
eleijonmarck and others added 3 commits February 24, 2023 10:02
This removes the migrationstatus state from the UI in favor of only
looking at the number of API keys to determine what to show to the user.
This simplifies the logic and makes less calls to the backend with each
page load. This was called both on the API keys page and the Service
accounts page.

- removes the state of migrationstatus from the UI
- removes the backend call
- removes the backend endpoint for migrationstatus
Co-authored-by: Karl Persson <kalle.persson@grafana.com>
@eleijonmarck eleijonmarck changed the title Service accounts: Refactor API keys tab Service accounts: Remove Add API keys buttons and remove one state of migrating for API keys tab Mar 1, 2023
@eleijonmarck eleijonmarck merged commit 9d6ab92 into main Mar 1, 2023
@eleijonmarck eleijonmarck deleted the eleijonmarck/serviceaccounts/hide-apikeys-tab-on-start branch March 1, 2023 15:34
ryantxu pushed a commit that referenced this pull request Mar 2, 2023
… migrating for API keys tab (#63411)

* add: hide apikeys tab on start

* make use of store method

* added hiding of apikeys tab for new org creation

* missing err check

* removed unused files

* implemennted fake to make tests run

* move check for globalHideApikeys from org to admin

* refactor to remove the fake

* removed unused method calls for interface

* Update pkg/services/serviceaccounts/manager/service.go

Co-authored-by: Alexander Zobnin <alexanderzobnin@gmail.com>

* Update pkg/services/serviceaccounts/manager/service.go

Co-authored-by: Alexander Zobnin <alexanderzobnin@gmail.com>

* remove the checkglobal method

* removed duplicate global set const

* add count of apikeys for performance

* remove apikeys adding in UI

* added back deleted file

* added comment on component

* changed wording and copy for hiding and migrating service accounts

* refactor: remove migrationstatus in front/backend

This removes the migrationstatus state from the UI in favor of only
looking at the number of API keys to determine what to show to the user.
This simplifies the logic and makes less calls to the backend with each
page load. This was called both on the API keys page and the Service
accounts page.

- removes the state of migrationstatus from the UI
- removes the backend call
- removes the backend endpoint for migrationstatus

* Update pkg/services/apikey/apikeyimpl/xorm_store.go

Co-authored-by: Karl Persson <kalle.persson@grafana.com>

* changes the contet to also be primary

* change id of version for footer component

---------

Co-authored-by: Alexander Zobnin <alexanderzobnin@gmail.com>
Co-authored-by: Karl Persson <kalle.persson@grafana.com>
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.

None yet

6 participants