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

NGINX Unit #992

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

NGINX Unit #992

wants to merge 41 commits into from

Conversation

gsfr
Copy link
Member

@gsfr gsfr commented Nov 14, 2017

This is work in progress. Help wanted.

Scope proposal:

  • Ability to run core's own test suite on NGINX Unit and Alpine Linux.
  • Ability to trivially bring up a fully functional core, including mongo, to be used for testing of other components, such as reaper or cli.

Comments and/or requirement definitions welcome.

TO DO

  • Revert Have Travis push to Docker Hub for this branch

@gsfr
Copy link
Member Author

gsfr commented Nov 14, 2017

@ambrussimon I would like to use the setup.py/requirements.txt pattern that you have established in reaper, but I don't know how to handle the necessary dependency_links. Any ideas?

Copy link
Contributor

@kofalt kofalt left a comment

Choose a reason for hiding this comment

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

Looks interesting!

Do you have any benchmarks or observed behavior?

requirements.txt Outdated
uwsgi==2.0.13.1
webapp2==2.5.2
WebOb==1.5.1
git+https://github.com/flywheel-io/[email protected]#egg=gears
Copy link
Contributor

Choose a reason for hiding this comment

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

I use this file for CI, so I would need an equivalent command. I'll want to try things out with this branch if it gets closer to merging.

Copy link
Member Author

Choose a reason for hiding this comment

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

We will make sure that you don't need any of that anymore. You should able to simply stand up a core and use it. No internal knowledge required.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, as long as the container checks itself on boot. Frequently I want to CI against a branch whose container (incl updated libraries) has not built yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point. We'll need to have images available for arbitrary branches of core to facilitate testing of interacting repos, such as reaper and cli. We typically build images for master and all tags. I think that's enough (rather than building for all branches) and keeps clutter at bay. Would that work for you, @kofalt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed this out of band - basically, as long as any CI container can update itself on boot to the tip of a branch (incl updated libraries) we're fine.

Dockerfile Outdated

RUN curl -L https://github.com/nginx/unit/archive/master.tar.gz | tar xz --strip-components 1
RUN ./configure --prefix=/usr/local --modules=lib --state=/var/local/unit --pid=/var/unit.pid --log=/var/log/unit.log \
&& ./configure python \
Copy link
Contributor

Choose a reason for hiding this comment

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

We should check the configure options, nginx-web (or whatever) has quite a few good ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

On it.

setup.py Outdated

dependency_links = [
'git+https://github.com/flywheel-io/[email protected]#egg=gears',
]
Copy link
Contributor

Choose a reason for hiding this comment

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

I take it this entry can't be in the above array?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not that I could tell, but @ambrussimon will look further into it.

@gsfr
Copy link
Member Author

gsfr commented Nov 14, 2017

@kofalt No benchmarks yet. Still at the 'getting it running' stage. We'll let you know when this is ready to be tested against.

@ryansanford
Copy link
Contributor

@gsfr Isn't the definition of this PR being ready that it's not a "Breaking Change" ?

@gsfr
Copy link
Member Author

gsfr commented Nov 17, 2017

@ryansanford Won't be a breaking chance from the user point of view, but will be for parts of our infra. Removing tag.

@nagem
Copy link
Contributor

nagem commented Nov 29, 2017

Remove abao

🎉🎉🎉🎉🎉🎉🎉🎉🎉

requirements.txt Outdated
@@ -1,16 +1,16 @@
django>=1.11.0,<1.12.0
elasticsearch==5.3.0
enum34==1.1.6
git+https://github.com/flywheel-io/[email protected]#egg=gears
ipython
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm guessing you want IPython for debugging, but it's such a quick install and I'd rather not bake it into the image.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just seeing where IPython came from. I'm taking it out.

requirements.txt Outdated
jsonschema==2.6.0
Markdown==2.6.5
markdown==2.6.5
Copy link
Member Author

Choose a reason for hiding this comment

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

The reason for the capitalization is to match the output of pip freeze, so that it can be copied verbatim.

@gsfr gsfr force-pushed the nginx-unit branch 2 times, most recently from e03df1f to b2cb26e Compare December 20, 2017 21:59
@gsfr
Copy link
Member Author

gsfr commented Jan 2, 2018

@ambrussimon nginx/unit#65 has been closed. We should now be able to drop your coverage endpoint workaround. However, it appears that Unit's introduction of HTTP Keep-Alive has broken our tests. The previous commit (nginx/unit@497faf1) is the last one that seems to work.

@ambrussimon
Copy link
Contributor

@gsfr Just rebased (fixed conflicts), no functional change.

@kofalt
Copy link
Contributor

kofalt commented Jan 31, 2018

Ref flywheel-io/sdk#80 for current endpoint status!

@gsfr
Copy link
Member Author

gsfr commented Feb 1, 2018

Thanks, @kofalt.

@ambrussimon Please take a look at the failing SDK tests. Getting those to pass would be a great milestone for this PR.

@kofalt
Copy link
Contributor

kofalt commented Feb 1, 2018

If Ambrus or anyone else has a look at playing with those, note the docs to help you out:
https://github.com/flywheel-io/sdk#testing

In particular, the -run flag and SdkTestDebug env variable may be of interest.

@gsfr
Copy link
Member Author

gsfr commented Mar 10, 2018

@ambrussimon In trying to get flywheel-io/sdk#80 going, I added upgrade_schema to dev+mongo.sh. Unfortunately, that seems to break some tests. Is it ok/desirable to have this? Can we fix the tests?

@gsfr gsfr removed the request for review from ryansanford May 11, 2018 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants