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

Url prefix #85

Merged
merged 4 commits into from
Apr 30, 2021
Merged

Url prefix #85

merged 4 commits into from
Apr 30, 2021

Conversation

mmacata
Copy link
Collaborator

@mmacata mmacata commented Apr 28, 2021

To make deployment independent of nginx, the openeo-grassgis-driver is - with this PR - able to add the API version prefix itself. As this is an api breaking change when used without nginx, a major release will be necessary afterwards.

@mmacata mmacata requested a review from metzm April 28, 2021 13:27
@mmacata mmacata mentioned this pull request Apr 29, 2021
Copy link
Collaborator

@metzm metzm left a comment

Choose a reason for hiding this comment

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

I'm not sure about hard-coding API_VERSION independent of well-known, otherwise fine.

@mmacata
Copy link
Collaborator Author

mmacata commented Apr 29, 2021

I moved the definition of API_VERSION to the WellKnown class. Please re-review @metzm

@mmacata mmacata requested a review from metzm April 29, 2021 10:11
Copy link
Collaborator

@metzm metzm left a comment

Choose a reason for hiding this comment

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

Much better now that the API version is defined only once and used throughout! Please merge.

@marcjansen
Copy link
Collaborator

Two questions:

  • In these lines in src/openeo_grass_gis_driver/well_known.py you define one URL_PREFIX. Does this mean that one instance of this driver will only ever be able to support one version of the API at the same time? I would have imagined that we may be supporting different versions from one server.
  • In app.py I read that CORS-support from flask may be shaky, and we therefore use an NGINX before the flask app. Has this been resolved? Dealing in an expected way with CORS is crucial from my understanding of interfaces

@mmacata
Copy link
Collaborator Author

mmacata commented Apr 29, 2021

Two questions:

  • In these lines in src/openeo_grass_gis_driver/well_known.py you define one URL_PREFIX. Does this mean that one instance of this driver will only ever be able to support one version of the API at the same time? I would have imagined that we may be supporting different versions from one server.

Yes that is the case but it has never been different. One state of the backend can only serve one version at a time. When we had multiple versions deployed, we did this by using different branches and let nginx do the reverse proxy. So this change is not a step towards more restriction but towards more transparency in my view. If we ever want to deploy multiple versions again, I would stick to the git branching approach.

  • In app.py I read that CORS-support from flask may be shaky, and we therefore use an NGINX before the flask app. Has this been resolved? Dealing in an expected way with CORS is crucial from my understanding of interfaces

The shaky appearance only appeared in combination with nginx and the case that 'Access-Control-Allow-Origin' '*' was forbidden by the API specification but more restrictive to the requesting client, e.g. Access-Control-Allow-Origin: http://client.org:8080. During that time we moved CORS from app.py to nginx because nginx would always add 'Access-Control-Allow-Origin' '*' unless explicitly hidden, but then also the header from python CORS was hidden. As now 'Access-Control-Allow-Origin' '*' is fine for a response and we want to get totally rid of nginx, it should be fine but will be tested of course.

@marcjansen
Copy link
Collaborator

@mmacata sounds all reasonable, thanks. Good to go fdrom my limited point of view.

@mmacata mmacata mentioned this pull request Apr 29, 2021
Copy link
Collaborator

@metzm metzm left a comment

Choose a reason for hiding this comment

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

The explanation of @mmacata for support of one single version makes sense.

@mmacata mmacata merged commit 022bedb into Open-EO:master Apr 30, 2021
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.

3 participants