fix(migration): 87d38ad83218 failing on upgrade

[villebro]

SUMMARY

On my local devenv today I ran into the issue described in #27896. I'm not sure why I hadn't run into this previously, but I believe it's because I had done a downgrade past this revision at some point, potentially triggering the erroneous perm state caused by the missing pvm in NEW_PVMS. After applying the change bug @ne1r0n proposed things worked out for me.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

• Has associated issue: fixes BUG: Can't flush None value found in collection Role.permissions #27896
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[villebro]

@sadpandajoe @michael-s-molina if there's another RC round for 4.1 I suggest cherrying this one in.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.75%. Comparing base (76d897e) to head (6f366b0).
Report is 750 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #30275       +/-   ##
===========================================
+ Coverage   60.48%   83.75%   +23.27%     
===========================================
  Files        1931      534     -1397     
  Lines       76236    38544    -37692     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    32284    -13830     
+ Misses      28017     6260    -21757     
+ Partials     2105        0     -2105     

Flag	Coverage Δ
hive	48.94% <ø> (-0.23%)	⬇️
javascript	?
mysql	76.72% <ø> (?)
postgres	76.81% <ø> (?)
presto	53.44% <ø> (-0.37%)	⬇️
python	83.75% <ø> (+20.27%)	⬆️
sqlite	76.26% <ø> (?)
unit	60.51% <ø> (+2.88%)	⬆️

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.

[sadpandajoe]

@sadpandajoe @michael-s-molina if there's another RC round for 4.1 I suggest cherrying this one in.

Sure. Really weird that these migrations are failing.

[villebro]

Btw, I'm wondering if our CI tests should do upgrade, init, downgrade, upgrade and finally init again? I think this may help catch issues related to down migrations.

[rusackas]

CC @eschutho since she's looking at E2E flows/upgrades currently.

[mistercrunch]

CI tests should do upgrade, init, downgrade, upgrade and finally init again

Seems like a good improvement to me, though I'd suggest putting it in its own GHA check as opposed to inline it wherever db upgrade is run (could slow down many tests and not send as clear of a signal when failing)