-
-
Notifications
You must be signed in to change notification settings - Fork 735
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
chore: delete project api tokens when last mapped project is removed #7503
chore: delete project api tokens when last mapped project is removed #7503
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
Currently lacks unit tests |
…cted) projects deleted
@@ -109,6 +111,8 @@ export const createProjectService = ( | |||
eventService, | |||
); | |||
|
|||
const apiTokenStore = new ApiTokenStore(db, eventBus, getLogger); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the project service use the api token store or the service? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine either way on this one. If we want to move more in the direction of talking between services and not stores then perhaps this is the way to go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Just to be clear: I don't know (and don't really have a strong opinion one way or the other 🤷🏼 ). Just came to think of it.
@@ -89,6 +91,7 @@ beforeAll(async () => { | |||
|
|||
environmentService = new EnvironmentService(stores, config, eventService); | |||
projectService = createProjectService(db.rawDatabase, config); | |||
apiTokenService = new ApiTokenService(stores, config, eventService); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making a note for myself to update this after merging this. We're updating the api token service to use the new create(Fake)?ApiTokenService
pattern (in merged pr #7519), so I can go and fix this one up too.
const projectTokens = allTokens.filter( | ||
(token) => | ||
(token.projects && | ||
token.projects.length === 1 && | ||
token.projects[0] === id) || | ||
token.project === id, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is essentially looking for tokens that only have a single project, and that project has id id
, right?
Not important, but is there a nicer way to do it? I think even with 24k tokens it should be fine, but how about something like:
const tokens = await this.apiTokenStore.getTokensForProject(id);
const toDelete = tokens.filter(token => token.project || token.projects.length === 1)
Or something like that? 🤷🏼 Not important, though. Oh, I also don't know if we have a method like that, soooo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find this.apiTokenStore.getTokensForProject
, I guess we could add this method though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it was just an idea I had (and def not a blocker). I think that method might be useful, but if this is the only place we do that, then ... eh 🤷🏼
…. else just delete projects
Co-authored-by: Thomas Heartman <[email protected]>
Removes API token when last project it is mapped to is deleted