Skip to content

fix: delete app also deletes metadata #1736

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

Merged
merged 15 commits into from
Nov 18, 2022

Conversation

eh-am
Copy link
Collaborator

@eh-am eh-am commented Nov 17, 2022

Add an ApplicationService that bridges ApplicationMetadataService and Storage.

The operations List, Get, Create are relayed to ApplicationMetadataService only.
Now Delete is relayed to both ApplicationMetadataService AND Storage. This is so that when one is deleting an App, it must be deleted from storage but ALSO be deleted from the metadata database. Otherwise the application will still be shown in the application dropdown.

Also moved handlers code to /api to keep things consistent with the new way we are managing these handlers.
Although there are no tests there (they are still in the admin package)

@github-actions
Copy link
Contributor

github-actions bot commented Nov 17, 2022

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
webapp/public/assets/app.js 492.45 KB (0%) 9.9 s (0%) 3.9 s (+5.42% 🔺) 13.7 s
webapp/public/assets/app.css 19.97 KB (0%) 400 ms (0%) 0 ms (+100% 🔺) 400 ms
webapp/public/assets/styles.css 9.56 KB (0%) 192 ms (0%) 0 ms (+100% 🔺) 192 ms
packages/pyroscope-flamegraph/dist/index.js 131.27 KB (0%) 2.7 s (0%) 1.8 s (+18.4% 🔺) 4.4 s
packages/pyroscope-flamegraph/dist/index.node.js 131.96 KB (0%) 2.7 s (0%) 707 ms (+29.16% 🔺) 3.4 s
packages/pyroscope-flamegraph/dist/index.css 8.29 KB (0%) 166 ms (0%) 0 ms (+100% 🔺) 166 ms

@codecov
Copy link

codecov bot commented Nov 17, 2022

Codecov Report

Base: 66.59% // Head: 66.21% // Decreases project coverage by -0.38% ⚠️

Coverage data is based on head (0433082) compared to base (661c205).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1736      +/-   ##
==========================================
- Coverage   66.59%   66.21%   -0.37%     
==========================================
  Files         169      169              
  Lines        5563     5560       -3     
  Branches     1260     1258       -2     
==========================================
- Hits         3704     3681      -23     
- Misses       1849     1869      +20     
  Partials       10       10              
Impacted Files Coverage Δ
.../pyroscope-flamegraph/src/Tooltip/TableTooltip.tsx 47.28% <0.00%> (-10.86%) ⬇️
...ackages/pyroscope-flamegraph/src/ProfilerTable.tsx 56.37% <0.00%> (-4.53%) ⬇️
packages/pyroscope-flamegraph/src/Toolbar.tsx 86.37% <0.00%> (-1.69%) ⬇️
...e-flamegraph/src/FlameGraph/FlameGraphRenderer.tsx 49.70% <0.00%> (-0.30%) ⬇️
...scope-flamegraph/src/Tooltip/FlamegraphTooltip.tsx 76.39% <0.00%> (ø)
packages/pyroscope-flamegraph/src/format/format.ts 73.22% <0.00%> (+0.49%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@eh-am eh-am marked this pull request as ready for review November 17, 2022 16:44
@eh-am eh-am marked this pull request as draft November 17, 2022 16:59
@Ivanikitin Ivanikitin added the backend Mostly go code label Nov 17, 2022
@eh-am eh-am changed the title chore: delete app also deletes metadata fix: delete app also deletes metadata Nov 18, 2022
@eh-am eh-am marked this pull request as ready for review November 18, 2022 11:12
Copy link
Collaborator

@kolesnikovae kolesnikovae left a comment

Choose a reason for hiding this comment

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

Looks just great :) I left a couple of notes – nothing really important but give them a look please

Copy link
Collaborator

@kolesnikovae kolesnikovae left a comment

Choose a reason for hiding this comment

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

LGTM!

@eh-am eh-am merged commit c485edd into main Nov 18, 2022
@eh-am eh-am deleted the chore/delete-app-also-deletes-metadata branch November 18, 2022 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Mostly go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants