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

Docker extension #46

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

lazaa32
Copy link
Contributor

@lazaa32 lazaa32 commented Dec 20, 2018

Overview

Docker extension for pywps-flask.
Updated readme.
Added parameters to config file.
Added requests for tests.
Updated isolation Dockerfile.

Related Issue / Discussion

Additional Information

Contribution Agreement

(as per https://github.com/geopython/pywps/blob/master/CONTRIBUTING.rst#contributions-and-licensing)

  • I'd like to contribute [feature X|bugfix Y|docs|something else] to PyWPS. I confirm that my contributions to PyWPS will be compatible with the PyWPS license guidelines at the time of contribution.
  • I have already previously agreed to the PyWPS Contributions and Licensing Guidelines

@lazaa32 lazaa32 mentioned this pull request Dec 20, 2018
2 tasks
README.rst Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
@jorgejesus
Copy link
Member

jorgejesus commented Dec 25, 2018

I have tested and tried to understand as maximum as I could, as far as could understand and IMHO. I have some comments concerning this PR

  • We should keep pywps-flask connected only to pywps master or tag branch, and not to and not to an branch that has different code. (This could be a problem when working and testing on branch)

  • There is a pull request on pywps Docker extension pywps#443 that first should be accepted and then specifically look at this PR and later look at pywps-flask

  • PR #443 on pywps will require a dock image to run the processes, think this will be better on the master of pywps

  • This pull request is properly done and has specific unnitests, I have set the configuration file to mode=docker but got to many connection error: https://pastebin.com/sWZgKRGP it seems that the requests shoulf be async (for what I understood from other docs)

My recommendations:

  • Wait until docker support is added to pywps
  • After present a PR for the pywps-flask using 4.2 or 4.4

My IMHO:

  • This is a very important work, and it has to be merged ASAP
  • Code is well structured and with specific unnitests (good work)

@lazaa32 Care to comment in case I am missing something or not having a clear idea of what is going on
@jachym I leave the final decision on how to proceed up to you.

@lazaa32
Copy link
Contributor Author

lazaa32 commented Dec 26, 2018

* We should keep `pywps-flask` connected only to  `pywps` master or tag branch, and not to and not to an branch that has different code. (This could be a problem when working and testing on branch)

* There is a pull request on pywps [geopython/pywps#443](https://github.com/geopython/pywps/pull/443) that first should be accepted and then specifically look at this PR and later look at  `pywps-flask`

I agree with you. I made both PRs to let you know that work both on pywps-flask and pywps is ready to test. Once geopython/pywps#443 is merged into master, there is no need for any other branch.

* PR #443 on pywps will require a dock image to run the processes, think this will be better on the master of pywps

* This pull request is properly done and has specific unnitests, I have set the configuration file to `mode=docker` but got to many connection error: https://pastebin.com/sWZgKRGP it seems that the requests shoulf be async (for what I understood from other docs)

Could you check whether a container is at least created and started after submitting a request, please?

My recommendations:

* Wait until docker support is added to pywps

* After present a PR for the pywps-flask using 4.2 or 4.4

+1

My IMHO:

* This is a very important work, and it has to be merged ASAP

It would be nice to get this merged by mid January. Right now I have a good wifi connection but later I can't promise I will have a chance to work on this constantly due to lack of electric power and wifi :(.

@jorgejesus
Copy link
Member

During my initial tests the docker containers where created. I will make a more extensive check to determine why they are not connecting

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.

2 participants