Skip to content
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

Remove package config from deployments response #7808

Merged

Conversation

johnnymetz
Copy link
Collaborator

@johnnymetz johnnymetz commented Mar 4, 2024

What does this PR do?

Demo

https://www.loom.com/share/da9c92ee615540e7972d936286c5eec2

Future Work

Checklist

  • Add tests
  • New files added to src/tsconfig.strictNullChecks.json (if possible)
  • Designate a primary reviewer: @grahamlangford

Copy link
Collaborator

@grahamlangford grahamlangford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple new files that can probably be added to the tsconfig.strictNullChecks.json. Let me know if you need help/context with that.

@twschiller twschiller added this to the 1.8.10 milestone Mar 4, 2024
@twschiller
Copy link
Contributor

twschiller commented Mar 5, 2024

Thanks @johnnymetz - a couple of callouts:

  • Functionality: the code is currently fetching all deployment mod definitions, not just updated deployments
  • Instead of a registry -> mod definition map, you need to keep the deployment + mod definition pairs, e.g., define a DeploymentModDefinitionPair type. It's required for correctness (see https://github.com/pixiebrix/pixiebrix-extension/pull/7808/files#r1511959208), and more readable than passing around two arguments that are linked

@grahamlangford
Copy link
Collaborator

@johnnymetz let me know if you need help with the merge conflict.

);
const { isAutoDeploying } = useAutoDeploy({
activatableDeployments,
installedExtensions: activeExtensions,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be using activate instead of installed as a naming convention (per the terminology guide). But updating these var names is low priority and there are many of them so I'm punting on it for now

Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Attention: Patch coverage is 92.30769% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 72.67%. Comparing base (6cb955b) to head (85d4103).

❗ Current head 85d4103 differs from pull request most recent head 8ebb897. Consider uploading reports for the commit 8ebb897 to get more accurate results

Files Patch % Lines
src/background/deploymentUpdater.ts 88.00% 3 Missing ⚠️
src/permissions/deploymentPermissionsHelpers.ts 33.33% 2 Missing ⚠️
src/data/service/apiVersioning.ts 80.00% 1 Missing ⚠️
...onConsole/pages/deployments/DeploymentsContext.tsx 96.15% 1 Missing ⚠️
src/testUtils/factories/deploymentFactories.ts 91.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7808      +/-   ##
==========================================
+ Coverage   72.65%   72.67%   +0.01%     
==========================================
  Files        1274     1276       +2     
  Lines       39909    39950      +41     
  Branches     7418     7424       +6     
==========================================
+ Hits        28997    29034      +37     
- Misses      10912    10916       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented Mar 6, 2024

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.

@johnnymetz johnnymetz enabled auto-merge (squash) March 6, 2024 21:46
@johnnymetz johnnymetz merged commit e9a0bfc into main Mar 6, 2024
16 checks passed
@johnnymetz johnnymetz deleted the feature/remove-package-config-from-deployments-response branch March 6, 2024 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants