-
-
Notifications
You must be signed in to change notification settings - Fork 737
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
[Gitar] Cleaning up stale flag: integrationEvents with value true #7940
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 |
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.
Seems to me the code removing was a tad over eager
properties: { | ||
integrationEvents: { | ||
type: 'array', | ||
description: 'A list of integration events.', | ||
items: { | ||
$ref: integrationEventSchema.$id, | ||
}, | ||
}, | ||
}, | ||
properties: {}, |
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.
Hmmm this doesn't seem right if we're keeping the feature and just removing the feature flag
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 guessing this is because the did a grep for integrationEvents
without considering whether something is a string or a "name".
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.
We do not do "grep" to transform the code. There are scenarios where it is OK to delete this entry, e.g.,
flags: {
integrationEvents: true,
},
We need to discuss and agree on when it is safe to delete and when it is not. @sjaanus
src/lib/routes/admin-api/addon.ts
Outdated
const normalizedLimit = | ||
Number(limit) > 0 && Number(limit) <= 100 ? Number(limit) : 50; | ||
const normalizedOffset = Number(offset) > 0 ? Number(offset) : 0; | ||
|
||
const integrationEvents = | ||
await this.integrationEventsService.getPaginatedEvents( | ||
id, | ||
normalizedLimit, | ||
normalizedOffset, | ||
); | ||
|
||
this.openApiService.respondWithValidation( | ||
200, | ||
res, | ||
integrationEventsSchema.$id, | ||
{ | ||
integrationEvents: serializeDates(integrationEvents), | ||
}, | ||
{}, |
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 looks a little weird, we're keeping the feature
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.
Same thing as the other one, I guess. It just looks for integrationEvents
, thinks "oh, I can delete that" and then also cleans up where it's used. We should see if we can fix that somehow.
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.
See the comment above!
…at were deleted in error
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.
Looks better now that we've added back a few deleted sections
This automated PR was generated by Gitar. View docs.