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

CentOS 7 Compatibility. Merged with dev branch. #67

Merged
merged 53 commits into from Jul 11, 2016

Conversation

knirbhay
Copy link
Contributor

@knirbhay knirbhay commented Jul 4, 2016

Travis build is running and all tests are passing for both OS (centos 7 + ubuntu trusty) inside docker.

I am getting one error log due to which build is failing in my fork

You have to provide either repo_token in .coveralls.yml, or launch via Travis or CircleCI

It looks like the https://coveralls.io/ requires all coverall calls to go from istresearch/scrapy-cluster fork.

Let me know if anything is required to do the merge.

Madison Bahmer and others added 30 commits April 7, 2016 15:18
…ng its the same issue as supervisord. Adding cronie as crond service for CentOS7 docker
@knirbhay
Copy link
Contributor Author

knirbhay commented Jul 6, 2016

Awesome. Will do these changes on my local machine and try to complete it asap.

@madisonb
Copy link
Collaborator

madisonb commented Jul 6, 2016

So this purposefully failed then, correct? the Travis build appears to have failed with the proper exit code 1 since you put some exceptions in the unit tests.

@knirbhay
Copy link
Contributor Author

knirbhay commented Jul 7, 2016

Yeap that's correct. But still I am verifying it with few more use cases, once verification is complete. I will let you know.

@knirbhay
Copy link
Contributor Author

knirbhay commented Jul 7, 2016

I am not 100% sure but it looks like docker has covered the returning exitcode in this Issue Add support for docker exec to return cmd exitStatus . After verifications, I can confirm our Travis setup is working fine without the forceful return of exit code.

@knirbhay
Copy link
Contributor Author

knirbhay commented Jul 7, 2016

Everything looks ok except following.

Even though I am running code outside the container. It looks like coveralls is pointing to the wrong dir.
Any Insight?

Installing collected packages: docopt, coveralls
Successfully installed coveralls-1.1 docopt-0.6.2
Submitting coverage to coveralls.io...
No source for /root/sc/lib/python2.7/site-packages/scutils-1.2.0.dev4-py2.7.egg/scutils/__init__.py
No source for /root/sc/lib/python2.7/site-packages/scutils-1.2.0.dev4-py2.7.egg/scutils/log_factory.py
No source for /root/sc/lib/python2.7/site-packages/scutils-1.2.0.dev4-py2.7.egg/scutils/method_timer.py
No source for /root/sc/lib/python2.7/site-packages/scutils-1.2.0.dev4-py2.7.egg/scutils/redis_queue.py
No source for /root/sc/lib/python2.7/site-packages/scutils-1.2.0.dev4-py2.7.egg/scutils/redis_throttled_queue.py
No source for /root/sc/lib/python2.7/site-packages/scutils-1.2.0.dev4-py2.7.egg/scutils/settings_wrapper.py
No source for /root/sc/lib/python2.7/site-packages/scutils-1.2.0.dev4-py2.7.egg/scutils/stats_collector.py
No source for /root/scrapy-cluster/crawler/crawling/__init__.py
No source for /root/scrapy-cluster/crawler/crawling/contextfactory.py
No source for /root/scrapy-cluster/crawler/crawling/custom_cookies.py
No source for /root/scrapy-cluster/crawler/crawling/distributed_scheduler.py
No source for /root/scrapy-cluster/crawler/crawling/items.py
No source for /root/scrapy-cluster/crawler/crawling/log_retry_middleware.py
No source for /root/scrapy-cluster/crawler/crawling/pipelines.py
No source for /root/scrapy-cluster/crawler/crawling/redis_dupefilter.py
No source for /root/scrapy-cluster/crawler/crawling/redis_retry_middleware.py
No source for /root/scrapy-cluster/crawler/crawling/spiders/__init__.py
No source for /root/scrapy-cluster/crawler/crawling/spiders/link_spider.py
No source for /root/scrapy-cluster/crawler/crawling/spiders/lxmlhtml.py
No source for /root/scrapy-cluster/crawler/crawling/spiders/redis_spider.py
No source for /root/scrapy-cluster/crawler/crawling/spiders/wandering_spider.py
No source for /root/scrapy-cluster/kafka-monitor/kafka_monitor.py
No source for /root/scrapy-cluster/kafka-monitor/plugins/__init__.py
No source for /root/scrapy-cluster/kafka-monitor/plugins/action_handler.py
No source for /root/scrapy-cluster/kafka-monitor/plugins/base_handler.py
No source for /root/scrapy-cluster/kafka-monitor/plugins/scraper_handler.py
No source for /root/scrapy-cluster/kafka-monitor/plugins/stats_handler.py
No source for /root/scrapy-cluster/kafka-monitor/plugins/zookeeper_handler.py
No source for /root/scrapy-cluster/redis-monitor/plugins/__init__.py
No source for /root/scrapy-cluster/redis-monitor/plugins/base_monitor.py
No source for /root/scrapy-cluster/redis-monitor/plugins/expire_monitor.py
No source for /root/scrapy-cluster/redis-monitor/plugins/info_monitor.py
No source for /root/scrapy-cluster/redis-monitor/plugins/kafka_base_monitor.py
No source for /root/scrapy-cluster/redis-monitor/plugins/stats_monitor.py
No source for /root/scrapy-cluster/redis-monitor/plugins/stop_monitor.py
No source for /root/scrapy-cluster/redis-monitor/plugins/zookeeper_monitor.py
No source for /root/scrapy-cluster/redis-monitor/redis_monitor.py
No source for /root/scrapy-cluster/utils/scutils/__init__.py
Coverage submitted!

@madisonb
Copy link
Collaborator

madisonb commented Jul 7, 2016

That is much cleaner, the exit 1's were because you chained everything together, but now that you split it up that works nicely.

I can accept running coveralls not in after_success. There are a number of Travis tickets related to only executing one time when the build matrix succeeds (vs every single time). According to this link the latest build ran works with sub builds, so if you add back in the coverage in the main script section to be ran within docker exec that is working.

Nice work so far! Thanks again for this we are very close!

@madisonb
Copy link
Collaborator

madisonb commented Jul 7, 2016

I have begun merging your branch into the main repo see here. Also added a commit to test the coverage report inside the script

@madisonb
Copy link
Collaborator

madisonb commented Jul 7, 2016

Ah so I think the issue is that the coverage report includes direct paths to the source files that it executed on. Because the Docker volume mount path is not the same on each side, it can't find the original source files (since the test scripts were ran inside the container at /root/scrapy-cluster and the original code lives at pwd we either need to find/replace or mount things at the same path. Here is an example of the .coverage output when ran locally:

...
"/Users/madisonb/Documents/Memex/scrapy-cluster/kafka-monitor/kafka_monitor.py": [3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60,
...

As you can see, it creates a report with full paths specified, and when we are outside the container we dont have the same full path.

@madisonb
Copy link
Collaborator

madisonb commented Jul 7, 2016

For when you see this, I got the centos build working on my branch but the ubuntu one is not. Also got coverage working for the build that passes. https://travis-ci.org/istresearch/scrapy-cluster/builds/143163692

I really just need /tmp/ mounted in the docker container but it doesn't seem to be happening on the ubuntu matrix build.

@knirbhay
Copy link
Contributor Author

knirbhay commented Jul 8, 2016

This is strange because centos worked well. Let me check today if I can mount both source locations in correct places.

@coveralls
Copy link

coveralls commented Jul 8, 2016

Coverage Status

Coverage decreased (-2.2%) to 62.461% when pulling 778675a on knirbhay:dev into d1f9999 on istresearch:dev.

@coveralls
Copy link

coveralls commented Jul 8, 2016

Coverage Status

Coverage decreased (-2.2%) to 62.461% when pulling 23bc767 on knirbhay:dev into d1f9999 on istresearch:dev.

@knirbhay
Copy link
Contributor Author

knirbhay commented Jul 8, 2016

This trick worked. I changed all the paths with ${PWD} so that it mounts the fs in container at the same location as host.

But there are few files related to virtualenv. I believe these reports need not go to the coverage report. Is there a way to ignore these files or should I create virtual env inside source folder?

No source for /root/sc/lib/python2.7/site-packages/scutils-1.2.0.dev4-py2.7.egg/scutils/__init__.py
No source for /root/sc/lib/python2.7/site-packages/scutils-1.2.0.dev4-py2.7.egg/scutils/log_factory.py
No source for /root/sc/lib/python2.7/site-packages/scutils-1.2.0.dev4-py2.7.egg/scutils/method_timer.py
No source for /root/sc/lib/python2.7/site-packages/scutils-1.2.0.dev4-py2.7.egg/scutils/redis_queue.py
No source for /root/sc/lib/python2.7/site-packages/scutils-1.2.0.dev4-py2.7.egg/scutils/redis_throttled_queue.py
No source for /root/sc/lib/python2.7/site-packages/scutils-1.2.0.dev4-py2.7.egg/scutils/settings_wrapper.py
No source for /root/sc/lib/python2.7/site-packages/scutils-1.2.0.dev4-py2.7.egg/scutils/stats_collector.py
Coverage submitted!
Couldn't find a repository matching this job.
'url'

@madisonb
Copy link
Collaborator

madisonb commented Jul 8, 2016

The coverage decreased because it apparently couldn't find those files. The coverage report from your latest commit has 31 files listed, where the old reports have 38 (thus, the average would be slightly different since those files are no longer there). We actually do want those files.

See this vs yours. This is because the sc virtualenv directory isnt mounted the same way the repo is. I say the best solution is to make the virtualenv not at ~/sc but inside of the mounted repo like ~/scrapy-cluster/sc. I may have time to do this today but I will drop a note in here if I can get to it.

@madisonb
Copy link
Collaborator

madisonb commented Jul 8, 2016

With the commit I added dc472a4 we are back to our expected coverage. I think this satisfies everything and if you are happy I am! Awesome!

@knirbhay
Copy link
Contributor Author

knirbhay commented Jul 9, 2016

Looks good to me.

@madisonb madisonb merged commit 23bc767 into istresearch:dev Jul 11, 2016
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.

None yet

4 participants