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 special note for EDD and EF #398

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add special note for EDD and EF #398

wants to merge 1 commit into from

Conversation

rkac-bw
Copy link

@rkac-bw rkac-bw commented Jul 29, 2024

🎟️ Tracking

📔 Objective

Add specific info in regards to implementing EDD process for Entity framework Only Databases with Code Migrations (Self host for example)

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation
    team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed
    issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@rkac-bw rkac-bw requested a review from a team as a code owner July 29, 2024 14:12
Copy link

Logo
Checkmarx One – Scan Summary & Details99c56dcc-ce4c-4f12-b2c1-b42937e06d81

No New Or Fixed Issues Found

@Hinton
Copy link
Member

Hinton commented Jul 29, 2024

Is this needed? To my knowledge we don't use EF migrations for cloud and unified has no requirement for EDD.

@withinfocus
Copy link
Contributor

Is this needed? To my knowledge we don't use EF migrations for cloud and unified has no requirement for EDD.

EF migrations are used for SM in the cloud right? I'd rather be consistent in our approach nonetheless though. It would be ideal to see a PR that includes EDD-prescribed changes both in a traditional DbUp migration script as well as an EF one.

Copy link
Contributor

@withinfocus withinfocus left a comment

Choose a reason for hiding this comment

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

Your lint is failing -- make sure that's running locally for you per the setup guide.

@@ -308,6 +308,26 @@ Prior to merging a PR please ensure that the database changes run well on the cu
version. We currently do not have an automated test suite for this and it’s up to the developers to
ensure their database changes run correctly against the currently released version.

## EF-Only Databases with Code Migrations
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## EF-Only Databases with Code Migrations
## EF-only databases with code migrations

@Hinton
Copy link
Member

Hinton commented Jul 29, 2024

No, SM uses regular migrations since using the EF framework for half our tables and hand written for the rest would be very difficult get running and even harder to follow.

For the delete scenario we should be fine having EF migrations that immediately delete the value. In cloud this means adding a migration for the next release to delete the value from the DB. If the value is required we make it not-required in the current release. This does mean we lose the ability to roll back using EF migrations, but that shouldn't occur on self-hosted instances.

There are more complex scenarios like column rename that requires some juggling with EF. And requires 3 deployments + 1 data migration.

  1. Add column new, update code to write both old and new. <- DB & code deploy.
  2. Run DB migration copying all old to new. <- DB
  3. Removing code references to old. <- Code
  4. Remove old from database. <- DB

@eliykat
Copy link
Member

eliykat commented Aug 8, 2024

Is there a consensus here? I need to drop some columns currently - which approach should I be using?

Regarding the PR itself - it would be useful to have a timeline showing both database frameworks side-by-side. e.g.


Example: dropping a column

Step # C# Entity MSSQL EF
1. No change Give the parameter a default value in all sprocs No change
Release
2. Delete the property No change Configure the column as a shadow property (no migration)
Release
3. No change Drop the column and remove corresponding sproc parameters (migration) Delete the shadow property (migration)

Maybe there is a better way to represent this.

Comment on lines +319 to +320
2. Keep that column in as a shadow property. Shadow properties are properties that are defined only in the EF model but are not defined in the C# classes. This can be particularly useful when you're transitioning columns in the database schema without immediately reflecting those changes in the code.

Copy link
Member

Choose a reason for hiding this comment

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

How do I do this? I had a look at https://learn.microsoft.com/en-us/ef/core/modeling/shadow-properties but it wasn't obvious how that maps to our implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants