-
Notifications
You must be signed in to change notification settings - Fork 22
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
#9257: fix heartbeat telemetry regression #9258
Conversation
Playwright test resultsDetails Open report ↗︎ Flaky testschrome › tests/pageEditor/addStarterBrick.spec.ts › Add starter brick to mod Skipped testschrome › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor |
// ModDefinition does not currently require version. | ||
blueprintVersion: | ||
modInstance.definition.metadata.version ?? | ||
normalizeSemVerString("0.0.0"), |
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.
nit: why "0.0.0" vs "1.0.0"?
It seems like we do this defaulting logic in a couple of different places so it might be worth looking into if we can make this a non-optional field upstream.
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.
nit: why "0.0.0" vs "1.0.0"?
So on the backend it's easier to see if it's defaulted or not
It seems like we do this defaulting logic in a couple of different places so it might be worth looking into if we can make this a non-optional field upstream.
I agree - see the future work ticket calling out fixing the upstream types. The change was too large to make on short turnaround
No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack. Do not edit this comment manually. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9258 +/- ##
==========================================
+ Coverage 74.24% 75.10% +0.85%
==========================================
Files 1332 1370 +38
Lines 40817 42302 +1485
Branches 7634 7892 +258
==========================================
+ Hits 30306 31769 +1463
- Misses 10511 10533 +22 ☔ View full report in Codecov by Sentry. |
What does this PR do?
Checklist
Future Work
ModDefinition.metadata.version
: Markversion
as required onModDefinition.metadata
#9259For more information on our expectations for the PR process, see the
code review principles doc