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

[CI] Configured travis builds #15 #18

Merged
merged 1 commit into from
Apr 6, 2020
Merged

Conversation

pandafy
Copy link
Member

@pandafy pandafy commented Apr 4, 2020

Closes #15

pip was erroring out due to incompatible requirements for cryptography module, so had to mention it in requirements.txt. See for more info

Kindly suggest any other way to mitigate this problem.
@nemesisdesign please turn on Travis and Coveralls for this repository.

@pandafy pandafy force-pushed the issues/15 branch 2 times, most recently from 3d31463 to 81a417e Compare April 4, 2020 22:41
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Thanks for picking it up fast.
See my comments.
I enabled travis and coveralls.

@@ -1,3 +1,4 @@
coveralls
Copy link
Member

Choose a reason for hiding this comment

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

not needed, openwisp-utils brings this dependency in: https://github.com/openwisp/openwisp-utils/blob/master/requirements-test.txt#L1

Copy link
Member Author

Choose a reason for hiding this comment

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

But that's in requirements-test.txt 😅

.travis.yml Outdated
- |
openwisp-utils-qa-checks \
--migration-path ./openwisp_monitoring/monitoring \
--migration-module monitoring
Copy link
Member

Choose a reason for hiding this comment

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

--migration-module monitoring monitoring device check

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 have update accordingly, took reference from openwisp-controller

.travis.yml Outdated
- pip install --no-cache-dir -U -r requirements-test.txt
- ./runflake8
- ./runisort
- pip install --no-cache-dir -U -r requirements.txt -r requirements-test.txt
Copy link
Member

Choose a reason for hiding this comment

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

install only requirements-test.txt, don't worry if it fails, I will help you debug it.

@pandafy pandafy force-pushed the issues/15 branch 5 times, most recently from 56bfe34 to ec88e25 Compare April 5, 2020 21:23
Copy link
Contributor

@nepython nepython left a comment

Choose a reason for hiding this comment

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

@TheOneAboveAllTitan I had a few suggestions.

  • The migration check is failing due to a bad PR of mine. I will take care of it sorry to cause any unnecessary trouble.
  • Consider using
    pip install -e .
    
    It will take care of the dependencies like pyasn1 and cryptography (though doesn't take care of coveralls somehow)
  • Also as a general practice I have observed that the build is tested across multiple Django versions, please consider it.

I have tried out the changes and the build is passing for me.
Hope it helps 😄

@nemesifier
Copy link
Member

nemesifier commented Apr 6, 2020

@nepython coveralls is pulled from openwisp-utils[qa] in requirements.txt: https://github.com/openwisp/openwisp-monitoring/blob/master/requirements-test.txt

I realized my assumption was wrong. But I thought that was the case so I'm sending a PR to fix that.

.travis.yml Outdated
- pip install -U pip wheel setuptools
- docker run -d --name influxdb -p 8086:8086 influxdb
- docker exec -it influxdb influx -execute 'CREATE DATABASE openwisp2'
- pip install -U pip wheel setuptools pyasn1
- pip install --no-cache-dir -U -r requirements-test.txt
Copy link
Member

Choose a reason for hiding this comment

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

let's remove --no-cache-dir for consistency with other modules. If you find this in other modules let me know so we can remove it. I added it at some point in the past in which I was going crazy with the travis cache. Then they added a way to clear the cache from the web app.

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 took reference from openwisp-firmware-upgrader and it is still present there. Should I create a PR for that?

.travis.yml Outdated
- pip install --no-cache-dir -U -r requirements-test.txt
- ./runflake8
- ./runisort

install:
- python setup.py -q develop
Copy link
Member

Choose a reason for hiding this comment

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

let's use pip install -e .

./openwisp_monitoring/device/migrations \
./openwisp_monitoring/monitoring/migrations \
./openwisp_monitoring/notifications/migrations" \
--migration-module "check device_monitoring monitoring notifications"
- coverage run --source=openwisp_monitoring runtests.py
Copy link
Member

Choose a reason for hiding this comment

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

I understood why this is failing, can you add pip install https://github.com/openwisp/openwisp-utils/tarball/master temporarily in before_install please?

requirements.txt Outdated
@@ -5,3 +5,4 @@ django-apptemplates>=1.4,<2.0
django-celery-email>=3.0.0,<3.1
djangorestframework>=3.11,<3.12
swapper
cryptography<2.9.0,>=2.4.0
Copy link
Member

Choose a reason for hiding this comment

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

not sure why this is needed, it should be already specified somewhere upstream. Can you try commenting it out and let the build run without it please?

@nepython
Copy link
Contributor

nepython commented Apr 6, 2020

@TheOneAboveAllTitan , I think runflake8 and runisort files can be safely removed, no longer required. 😄

Update: They might now be required, just read openwisp/openwisp-docs#100 though it is about introducing a new script unsure if these can be removed safely.

@pandafy
Copy link
Member Author

pandafy commented Apr 6, 2020

Thank you @nepython for your suggestions. I have applied these suggestions. The build is passing now.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

yess 👍

@nemesifier nemesifier merged commit df70075 into openwisp:master Apr 6, 2020
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.

[ci] Setup travis build with code coverage
3 participants