-
-
Notifications
You must be signed in to change notification settings - Fork 656
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: remove new frontend api feature flag #6906
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
@@ -1,155 +0,0 @@ | |||
import { |
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 was only testing the diff and had no tests left.
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.
✅ Code Health Quality Gates: OK
- Improving Code Health: 1 findings(s) ✅
FRONTEND_API_REPOSITORY_CREATED, | ||
PROXY_REPOSITORY_CREATED, | ||
} from '../../metric-events'; | ||
import { FRONTEND_API_REPOSITORY_CREATED } from '../../metric-events'; |
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.
✅ No longer an issue: Code Duplication
The module no longer contains too many functions with similar structure
return client; | ||
} | ||
|
||
private async createNewClientForFrontendApiToken( | ||
private async createClientForFrontendApiToken( |
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.
Looking at lines 172 and 182 in this service, we did not do this for new clients during beta phase.
I renamed the newClients
to clients
and it auto-resolved, but I think it is correct to still have delete and destroy functions on the new clients.
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.
✅ Code Health Quality Gates: OK
- Improving Code Health: 1 findings(s) ✅
} else if ( | ||
this.config.flagResolver.isEnabled('returnGlobalFrontendApiCache') | ||
) { | ||
toggles = |
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.
just to make sure, is this the path that we return after the flag removal?
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.
yes
this.config.flagResolver.isEnabled('returnGlobalFrontendApiCache') | ||
) { | ||
toggles = | ||
await this.services.frontendApiService.getNewFrontendApiFeatures( |
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 assuming you renamed this to getFrontendApiFeatures so that we don't have old/new naming
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.
yes
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.
To be on the safe side I'd manually test this before merging
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.
✅ Code Health Quality Gates: OK
- Improving Code Health: 1 findings(s) ✅
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.
❌ Code Health Quality Gates: FAILED
- Declining Code Health: 5 findings(s) 🚩
- Improving Code Health: 3 findings(s) ✅
@@ -768,6 +780,7 @@ test('should filter features by environment', async () => { | |||
enabled: true, | |||
strategies: [{ name: 'default', parameters: {} }], | |||
}); | |||
await frontendApiService.refreshData(); |
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.
❌ Getting worse: Large Method
'should filter features by environment' increases from 120 to 121 lines of code, threshold = 70
@@ -636,6 +647,7 @@ test('should filter features by project', async () => { | |||
enabled: true, | |||
strategies: [{ name: 'default', parameters: {} }], | |||
}); | |||
await frontendApiService.refreshData(); |
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.
❌ Getting worse: Large Method
'should filter features by project' increases from 109 to 110 lines of code, threshold = 70
@@ -1256,6 +1131,7 @@ test('should NOT evaluate disabled strategies when returning toggles', async () | |||
], | |||
}); | |||
|
|||
await frontendApiService.refreshData(); |
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.
❌ Getting worse: Large Method
'should NOT evaluate disabled strategies when returning toggles' increases from 98 to 99 lines of code, threshold = 70
@@ -161,6 +160,7 @@ test('should allow requests with a token secret alias', async () => { | |||
alias: randomId(), | |||
environment: envB, | |||
}); | |||
await frontendApiService.refreshData(); |
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.
❌ Getting worse: Large Method
'should allow requests with a token secret alias' increases from 82 to 83 lines of code, threshold = 70
@@ -1144,6 +1018,7 @@ test('should not return all features', async () => { | |||
}, | |||
], | |||
}); | |||
await frontendApiService.refreshData(); |
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.
❌ Getting worse: Large Method
'should not return all features' increases from 71 to 72 lines of code, threshold = 70
@@ -12,18 +12,16 @@ import { | |||
} from '../../types/models/api-token'; | |||
import { startOfHour } from 'date-fns'; | |||
import { | |||
FEATURE_UPDATED, |
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.
✅ No longer an issue: Code Duplication
The module no longer contains too many functions with similar structure
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.
❌ Code Health Quality Gates: FAILED
- Declining Code Health: 5 findings(s) 🚩
- Improving Code Health: 3 findings(s) ✅
@@ -12,18 +12,16 @@ import { | |||
} from '../../types/models/api-token'; | |||
import { startOfHour } from 'date-fns'; | |||
import { | |||
FEATURE_UPDATED, |
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.
✅ Getting better: Lines of Code in a Single File
The lines of code decreases from 1236 to 1139, improve code health by reducing it to 1000
The flag has been 100% for a bit now, we need to prepare for GA.