Ouranosinc / Magpie

AuthN/AuthZ services
https://pavics-magpie.readthedocs.io
Apache License 2.0
1 stars 5 forks source link

allow service registration forced update of type field #590

Closed fmigneault closed 11 months ago

fmigneault commented 11 months ago

relates to https://github.com/bird-house/birdhouse-deploy/pull/348

codecov[bot] commented 11 months ago

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (4e6aa9c) 80.84% compared to head (027d107) 80.82%. Report is 3 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #590 +/- ## ========================================== - Coverage 80.84% 80.82% -0.03% ========================================== Files 73 73 Lines 10185 10188 +3 Branches 1822 1823 +1 ========================================== Hits 8234 8234 - Misses 1629 1631 +2 - Partials 322 323 +1 ``` | [Files](https://app.codecov.io/gh/Ouranosinc/Magpie/pull/590?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ouranosinc) | Coverage Δ | | |---|---|---| | [magpie/register.py](https://app.codecov.io/gh/Ouranosinc/Magpie/pull/590?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ouranosinc#diff-bWFncGllL3JlZ2lzdGVyLnB5) | `48.37% <0.00%> (-0.25%)` | :arrow_down: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

fmigneault commented 11 months ago

@mishaschwartz

I don't think this needs to be addressed in this PR but I'm a little concerned that this can "break the Magpie instance if those definitions are not compatible".

I did a little experimenting and it looks like an incompatible resource definition will cause a 500 error for certain routes but won't completely break the whole Magpie instance which is good.

Indeed. This is what I meant by "break". The code assumes that valid combinations of services/resources/permissions were pre-validated by the API/UI/registration script beforehand. If items are combined in a way that is invalid, it won't revalidate everything each time, because that would slow down way too much the execution.

This is mostly why the svc.type = svc_type was not applied before. I'm willing to let it be updated for the force_update case, but it is a "due at your own risk" kind of operation. In most cases, this will break, but for situations such as merging the individual parts of geoserver[wfs|wms|wps|api] into geoserver, that would work (but not the other way around!).

Maybe we should add a line about how to recover in case a breakage occurs.

For this, I think it is more a case of backing up your DB, try it, and revert as needed.