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

Add credentialSecret to snapshot model and update logic to use it #3649

Merged
merged 5 commits into from
Apr 2, 2024

Conversation

knolleary
Copy link
Member

Fixes #3644

Description

This adds credentialSecret to the snapshot model so we no longer depend on the parent of the snapshot existing. This makes it easier to reason about the snapshots as they get moved around.

The db migration will populate the new column with the appropriate value from with the Device or ProjectSettings tables. I've tested this migration in both sqlite and postgres.

I have looked at many of the pipeline tests that relate to snapshots. They checked the result had a credentials property, but didn't try to validate the value. I've updated the tests in a few places to validate the credentials are properly encrypted with the expected key.

I've also done various bits of manual testing with rollbacks, instance-assigned devices, app-assigned devices, pipeline deploys to instances and devices/device groups.

@knolleary knolleary added area:migration Involves a database migration deploy:pr labels Mar 27, 2024
@knolleary
Copy link
Member Author

Am going to spin up a test env to do some more pointing and clicking before we merge, but please continue the review of the code.

Copy link

codecov bot commented Mar 27, 2024

Codecov Report

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

Project coverage is 41.88%. Comparing base (830cef9) to head (8e5cf18).

Files Patch % Lines
...s/20240327-01-add-credentialSecret-to-snapshots.js 85.71% 1 Missing ⚠️
forge/routes/api/deviceLive.js 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3649      +/-   ##
==========================================
- Coverage   41.88%   41.88%   -0.01%     
==========================================
  Files         628      629       +1     
  Lines       24417    24413       -4     
  Branches     5977     5973       -4     
==========================================
- Hits        10228    10225       -3     
+ Misses      14189    14188       -1     
Flag Coverage Δ
backend 78.58% <88.88%> (-0.02%) ⬇️
frontend 1.91% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@knolleary
Copy link
Member Author

Hitting a postgres-only test failure. Can recreate locally on pg, but on sqlite. Investigating...

@knolleary
Copy link
Member Author

Test fixed.

@Steve-Mcl
Copy link
Contributor

code and tests read fine but i am pulling this branch for a quick run through.

Copy link
Contributor

@Steve-Mcl Steve-Mcl left a comment

Choose a reason for hiding this comment

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

code review and local testing with app device and other instance snapshot deploys 👍

@Steve-Mcl
Copy link
Contributor

@knolleary Approved. Had to update branch and so tests are re-running. Merge when tests pass.

@Steve-Mcl
Copy link
Contributor

Steve-Mcl commented Apr 2, 2024

@knolleary now that #3645 is merged, we can merge this.

#3645 had migration "20240325-01-add-teamTypeScope-to-FlowTemplates.js" which has a date 2 days prior to this PR "20240327-01-add-credentialSecret-to-snapshots.js"

I've merged main to this and will merge as soon as tests pass unless you shout up

@knolleary
Copy link
Member Author

@Steve-Mcl all good - thanks

@Steve-Mcl Steve-Mcl merged commit 8e5d441 into main Apr 2, 2024
10 checks passed
@Steve-Mcl Steve-Mcl deleted the 3644-snapshot-cred-handling branch April 2, 2024 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:migration Involves a database migration deploy:pr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate credentialSecret into Snapshot model
2 participants