-
Notifications
You must be signed in to change notification settings - Fork 224
Webhook subscriptions shouldnt block dev #6282
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
Webhook subscriptions shouldnt block dev #6282
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
1ed4170 to
716cb5c
Compare
|
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
Coverage report
Test suite run success3160 tests passing in 1330 suites. Report generated by 🧪jest coverage report action from 42ad5af |
| const extensions = (await developerPlatformClient.appExtensionRegistrations(remoteApp)).app.extensionRegistrations | ||
| if (!extensions.every((extension) => extension.id)) { | ||
| if ( | ||
| !extensions |
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 want to confirm we can remove this code later right? (since this is a new "Consistency violation" we should have a plan for end-of-life)
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 whole function will be removed as part of the migration cleaning, it was only added to show a temporary banner
pt2pham
left a comment
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.
+1 to Mitch that we need to note this down as tech debt to remove
a1c1181 to
c07b233
Compare
716cb5c to
42ad5af
Compare
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/public/node/monorail.d.ts@@ -112,10 +112,6 @@ export interface Schemas {
app_web_framework?: Optional<string>;
app_web_frontend_any?: Optional<boolean>;
app_web_frontend_count?: Optional<number>;
- cmd_theme_timings?: Optional<string>;
- cmd_theme_errors?: Optional<string>;
- cmd_theme_retries?: Optional<string>;
- cmd_theme_events?: Optional<string>;
env_ci?: Optional<boolean>;
env_ci_platform?: Optional<string>;
env_device_id?: Optional<string>;
|

WHY are these changes introduced?
Fixes an issue where webhook subscriptions were incorrectly blocking the dev command due to missing UIDs.
WHAT is this pull request doing?
Modifies the
blockIfMigrationIncompletefunction to ignore webhook subscriptions when checking if extensions have assigned UIDs. Webhook subscriptions do need UIDs but shouldn't block the dev command from proceeding.How to test your changes?
Measuring impact
How do we know this change was effective? Please choose one:
Checklist