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

Setup Experience Migration #22405

Merged
merged 9 commits into from
Sep 30, 2024

Conversation

dantecatalfamo
Copy link
Member

@dantecatalfamo dantecatalfamo commented Sep 25, 2024

#22375

  • Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.
    See Changes files for more information.
  • If database migrations are included, checked table schema to confirm autoupdate
  • For database migrations:
    • Checked schema for all modified table for columns that will auto-update timestamps during migration.
    • Confirmed that updating the timestamps is acceptable, and will not cause unwanted side effects.
    • Ensured the correct collation is explicitly set for character columns (COLLATE utf8mb4_unicode_ci).

@dantecatalfamo dantecatalfamo changed the title Dump schema Setup Experience Migration Sep 25, 2024
Copy link

codecov bot commented Sep 26, 2024

Codecov Report

Attention: Patch coverage is 77.46479% with 16 lines in your changes missing coverage. Please review.

Project coverage is 65.22%. Comparing base (7fa51b6) to head (42b6054).
Report is 1 commits behind head on feat-setup-experience.

Files with missing lines Patch % Lines
...s/20240925112748_AddSetupExperienceResultsTable.go 83.05% 6 Missing and 4 partials ⚠️
...11236_AddInstallDuringSetupToSoftwareInstallers.go 50.00% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@                    Coverage Diff                    @@
##           feat-setup-experience   #22405      +/-   ##
=========================================================
+ Coverage                  65.21%   65.22%   +0.01%     
=========================================================
  Files                       1496     1498       +2     
  Lines                     117157   117228      +71     
  Branches                    3480     3480              
=========================================================
+ Hits                       76399    76465      +66     
+ Misses                     33640    33638       -2     
- Partials                    7118     7125       +7     
Flag Coverage Δ
backend 66.50% <77.46%> (+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.

return fmt.Errorf("failed to add install_during_setup to vpp_apps_teams: %w", err)
}

_, err = tx.Exec(`ALTER TABLE host_script_results ADD install_during_setup BOOL NOT NULL DEFAULT false`)
Copy link
Member Author

Choose a reason for hiding this comment

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

It might be worth adding a foreign key column to host_script_results pointing to the new setup experience scripts table in the other file instead of a bool

Copy link
Member Author

Choose a reason for hiding this comment

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

The upside is that it's clear which setup script it belongs to without searching the other table for the row that references this row. The downside is that once the script is deleted and we set the foreign key in the host_script_results table to NULL, we no longer know that the script was run by a setup script, and it now just looks like an ad-hoc script run

Copy link
Member Author

Choose a reason for hiding this comment

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

@roperzh Going with foreign key for now, would appreciate any opinions on this

@dantecatalfamo dantecatalfamo marked this pull request as ready for review September 27, 2024 15:19
@dantecatalfamo dantecatalfamo requested a review from a team as a code owner September 27, 2024 15:19
@dantecatalfamo dantecatalfamo merged commit 1cac6ff into feat-setup-experience Sep 30, 2024
15 of 16 checks passed
@dantecatalfamo dantecatalfamo deleted the setup-experience-db-migration branch September 30, 2024 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants