Skip to content

feat(accounts): enhance auth account data structure #1139

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

Merged
merged 39 commits into from
Jun 2, 2024

Conversation

setchy
Copy link
Member

@setchy setchy commented May 23, 2024

  • Introduce new accounts type that unlocks the possibility of authenticating one or more accounts at once, across any combination of support auth methods (personal access token or oauth app) and across any platforms (github cloud or github enterprise server)

  • Update logic to support new account structures

  • Implemented a legacy auth/account migration process to help seamlessly transition existing authenticated users.

Note: there is a handful of follow-up PRs required in order to expand functionality beyond "at most one account of each auth method"

@setchy setchy marked this pull request as draft May 23, 2024 11:19
@setchy setchy added this to the Release 5.7.0 milestone May 23, 2024
@setchy setchy added the enhancement New feature or enhancement to existing functionality label May 23, 2024
@setchy setchy changed the title refactor: introduce new account types feat(accounts): new account system May 23, 2024
@setchy setchy marked this pull request as ready for review May 24, 2024 14:56
@setchy
Copy link
Member Author

setchy commented May 24, 2024

@afonsojramos @bmulholland - I'd appreciate if you could please test this branch locally when you have a moment.

@setchy setchy changed the title feat(accounts): new account system feat(accounts): enhance auth account data structure May 25, 2024
@setchy setchy marked this pull request as draft May 25, 2024 21:46
@setchy setchy marked this pull request as ready for review May 31, 2024 15:03
@setchy
Copy link
Member Author

setchy commented May 31, 2024

OK - @bmulholland @afonsojramos - I feel like this is ready for your eyes and feedback.

It largely focuses on the new account data structure and legacy account migration logic.

If all goes well, you'll notice no difference :)

@setchy
Copy link
Member Author

setchy commented May 31, 2024

👀 looking into the coverage drop

@brandonweiss
Copy link
Contributor

@setchy I think you might have tagged the wrong person 😬

@bmulholland
Copy link
Collaborator

Any chance there's more that could be pulled out? Not sure when I'll have time for an 800 line review :)

@setchy
Copy link
Member Author

setchy commented May 31, 2024

It's as lean as i think makes sense...

It's not as scary as it may appear

@setchy
Copy link
Member Author

setchy commented May 31, 2024

@bmulholland - thinking more about this... I could do the following PRs

  1. new unused types
  • contains the new Account types
  • set sensible defaults
  • leave unused
  1. migration utility
  • logic to migrate legacy accounts to the new account structure
  1. switch all API calls to use the new account object from 1 above.

lmk what you think.

Copy link
Member

@afonsojramos afonsojramos left a comment

Choose a reason for hiding this comment

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

Managed to find the time and it looks good to me code wise! Will test locally in the next few minutes!

@setchy
Copy link
Member Author

setchy commented May 31, 2024

Thanks @afonsojramos - anything explode haha

@afonsojramos
Copy link
Member

@setchy When I opened I didn't have the live one open. And now if I try to open the live one it doesn't open either.
image
image

@afonsojramos
Copy link
Member

If I open both, "live" one first, then local, the local one show the login screen.
image
While the live one stays on the white screen.

@setchy
Copy link
Member Author

setchy commented Jun 1, 2024

🥹

@setchy
Copy link
Member Author

setchy commented Jun 1, 2024

The stacktrace might be unrelated to this PR... similar error reported in #1166

EDIT: pr raised #1167

@afonsojramos
Copy link
Member

Looking good!

@afonsojramos afonsojramos merged commit d75fb82 into main Jun 2, 2024
7 checks passed
@afonsojramos afonsojramos deleted the refactor/new-account-type branch June 2, 2024 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or enhancement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants