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

allow service registration forced update of type field #590

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

fmigneault
Copy link
Collaborator

@fmigneault fmigneault self-assigned this Oct 3, 2023
@github-actions github-actions bot added the doc Documentation improvements or building problem label Oct 3, 2023
Copy link
Collaborator

@mishaschwartz mishaschwartz left a comment

Choose a reason for hiding this comment

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

This looks good to me.

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.

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

  • If updating the service type results in unexpected behaviour due to incompatible Resource or Permission definitions, please manually delete or update the problematic definitions from the Magpie database.

I know it's not a very user-friendly solution but it at least gives the user a recommended course of action.

@codecov
Copy link

codecov bot commented Oct 3, 2023

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
@@            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 Coverage Δ
magpie/register.py 48.37% <0.00%> (-0.25%) ⬇️

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

@fmigneault
Copy link
Collaborator Author

fmigneault commented Oct 3, 2023

@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.

@fmigneault fmigneault merged commit 6cf49b1 into master Oct 3, 2023
28 of 30 checks passed
@fmigneault fmigneault deleted the register-update-svc-type branch October 3, 2023 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Documentation improvements or building problem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants