/os-conductor

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

Use DPP runner instead of API loader #28

Merged
merged 20 commits into from Apr 17, 2018

Conversation

Reviewers

@brew brew

@pwalsh pwalsh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@akariv @brew @pwalsh
@akariv
Contributor

akariv commented Mar 12, 2018 •
edited

DPP Runner (https://github.com/akariv/dpp-runner) is a small utility library for running a single pipeline/source spec in a temporary workspace and as a separate thread.
You can submit these specs to it, and it will call a callback function with status updates.

The modifications in this PR:

  • Remove the status callback from os conductor (the one that API loader used to call), and repurposes the code to serve as the dpp-runner callback.
  • Modifies the package/upload handler, to create a fiscal source-spec from the provided datapackage and use that spec as an input to dpp-runner
  • Modifications to the Dockerfile to allow all that goodness to actually work in a single container.

(there are still a couple of tests failing, will take care of them)

@akariv akariv added the in progress label Mar 12, 2018

@akariv akariv requested a review from brew Mar 12, 2018

@akariv
bump dpp-fiscal
Loading status checks…
006c520
@brew

brew reviewed Mar 12, 2018
View changes

A few comments and questions inline.

conductor/blueprints/package/blueprint.py
@@ -35,8 +35,7 @@ def upload():
abort(400)
if jwt is None:
abort(403)
callback = os_conductor+url_for('.callback')

This comment has been minimized.

Show comment
Hide comment
@brew

brew Mar 12, 2018

Member

I think this means you can get rid of the os_conductor variable on L16, and the OS_CONDUCTOR_URL env var completely (and its mention in the README), as it's only there to help define the callback url.
Copy link
@brew

brew Mar 12, 2018

Member

I think this means you can get rid of the os_conductor variable on L16, and the OS_CONDUCTOR_URL env var completely (and its mention in the README), as it's only there to help define the callback url.

This comment has been minimized.

Show comment
Hide comment
@akariv

akariv Mar 12, 2018

Contributor

will do
Copy link
@akariv

akariv Mar 12, 2018

Contributor

will do

This comment has been minimized.

Show comment
Hide comment
@brew

brew Mar 12, 2018

Member

And OS_API_URL too :)
Copy link
@brew

brew Mar 12, 2018

Member

And OS_API_URL too :)

This comment has been minimized.

Show comment
Hide comment
@akariv

akariv Mar 20, 2018

Contributor

done
Copy link
@akariv

akariv Mar 20, 2018

Contributor

done
Show outdated Hide outdated conductor/blueprints/package/controllers.py
Show outdated Hide outdated conductor/blueprints/package/controllers.py
requirements.txt
os-package-registry>=0.0.13
datapackage-pipelines-fiscal>=1.0.7
datapackage-pipelines-aws
datapackage-pipelines[speedup]

This comment has been minimized.

Show comment
Hide comment
@brew

brew Mar 12, 2018

Member

I've not used that square-brackets syntax before. Learned something new!
Copy link
@brew

brew Mar 12, 2018

Member

I've not used that square-brackets syntax before. Learned something new!

This comment has been minimized.

Show comment
Hide comment
@akariv

akariv Mar 20, 2018

Contributor

👍
Copy link
@akariv

akariv Mar 20, 2018

Contributor

👍
Show outdated Hide outdated Dockerfile
Dockerfile
postgresql-dev \
build-base \
bash \
curl

This comment has been minimized.

Show comment
Hide comment
@brew

brew Mar 12, 2018

Member

We need bash and curl to make the entrypoint.sh script work.
Copy link
@brew

brew Mar 12, 2018

Member

We need bash and curl to make the entrypoint.sh script work.

This comment has been minimized.

Show comment
Hide comment
@akariv

akariv Mar 12, 2018

Contributor

okay
Copy link
@akariv

akariv Mar 12, 2018

Contributor

okay

This comment has been minimized.

Show comment
Hide comment
@akariv

akariv Mar 20, 2018

Contributor

bash and curl - check!
Copy link
@akariv

akariv Mar 20, 2018

Contributor

bash and curl - check!
Show outdated Hide outdated Dockerfile

akariv added some commits Mar 20, 2018

@akariv
Merge branch 'master' into feature/dpp-runner
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
f790285
@akariv
CR changes
Loading status checks…
f1cb961
@akariv
Add leveldb dependency
Loading status checks…
3426155
@akariv
lint
Loading status checks…
ebd7d72
@akariv
tests
Loading status checks…
e09baba
@akariv
Fix compose file
Loading status checks…
002fd79
@akariv

This comment has been minimized.

Show comment
Hide comment
@akariv

akariv Mar 20, 2018

Contributor

@brew fixed CR, tests and Docker.
Contributor

akariv commented Mar 20, 2018

@brew fixed CR, tests and Docker.

@akariv akariv closed this Mar 20, 2018

@akariv akariv reopened this Mar 20, 2018

@akariv akariv added in progress and removed in progress labels Mar 20, 2018

.travis.yml
@@ -25,6 +25,7 @@ before_install:
install:
# Will build local Dockerfile as part of ci-run (docker-compose up)
- sudo apt-get install libleveldb-dev libleveldb1

This comment has been minimized.

Show comment
Hide comment
@brew

brew Mar 21, 2018

Member

What's this used by? Do we need it in the Dockerfile instead, or is it just for Travis?
Copy link
@brew

brew Mar 21, 2018

Member

What's this used by? Do we need it in the Dockerfile instead, or is it just for Travis?

This comment has been minimized.

Show comment
Hide comment
@akariv

akariv Mar 21, 2018

Contributor

It's actually already in the dockerfile - these are native libraries used by the speedup extension for dpp.
Copy link
@akariv

akariv Mar 21, 2018

Contributor

It's actually already in the dockerfile - these are native libraries used by the speedup extension for dpp.
@brew

This comment has been minimized.

Show comment
Hide comment
@brew

brew Mar 21, 2018

Member

Nice! A joy to get rid of more env vars :D
Member

brew commented Mar 21, 2018

Nice! A joy to get rid of more env vars :D
Dockerfile
RUN update-ca-certificates
RUN apk add --update --no-cache libpq postgresql-dev libffi libffi-dev bash curl libstdc++ nodejs
RUN apk add --update --no-cache --virtual=build-dependencies build-base
RUN apk --repository http://dl-3.alpinelinux.org/alpine/edge/testing/ --update add leveldb leveldb-dev

This comment has been minimized.

Show comment
Hide comment
@akariv

akariv Mar 21, 2018

Contributor

@brew here ^
Copy link
@akariv

akariv Mar 21, 2018

Contributor

@brew here ^
@brew
Add env vars to tox for tests
Loading status checks…
c10f53f

@brew brew merged commit c10f53f into master Apr 17, 2018
3 checks passed

3 checks passed

ci/dockercloud Your image was successfully built in Docker Cloud
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 75.83%
Details

@brew brew removed the in progress label Apr 17, 2018

@brew brew deleted the feature/dpp-runner branch Apr 17, 2018

@brew brew referenced this pull request Apr 19, 2018

Closed

Package loader no longer obfuscates email or sets 'private' to default #1350

brew added a commit to openspending/openspending that referenced this pull request Apr 23, 2018

@brew
Remove os-api-loader 
As part of openspending/os-api#18 and openspending/os-conductor#28
833572f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment