-
Notifications
You must be signed in to change notification settings - Fork 112
Travis-CI + Coveralls #46
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
Conversation
|
I suggest renaming the |
|
For this we need
linux-system-roles/test-harness@3dbfe29
to be deployed or we will break our CI.
2018-05-14 17:51 GMT+02:00 pcahyna <notifications@github.com>:
… I suggest renaming the test/ directory to tests/ while you are moving
things around. tests/ is in the default role directory structure (try ansible-galaxy
init).
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#46 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAMEWP32TCTX6Kxu1nga0USSzSmzYNzJks5tyafwgaJpZM4T99gI>
.
--
Till Maas
Ansible RHEL Networking System Role Maintainer
Red Hat GmbH
|
It has been deployed since a long time. |
|
I mean the new code from the pull request, is it already running?
2018-05-14 18:10 GMT+02:00 pcahyna <notifications@github.com>:
… For this we need ***@***.***
<linux-system-roles/test-harness@3dbfe29> to be
deployed or we will break our CI.
It has been deployed since a long time.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#46 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAMEWJ80TRccECInlyh-RrMPYXpMdIHsks5tyayOgaJpZM4T99gI>
.
--
Till Maas
Ansible RHEL Networking System Role Maintainer
Red Hat GmbH
|
|
That's the point. It was deployed before being merged. I.e. the state of the CI does not correspond to the GitHub sources. Confusing, huh? |
|
So it broke our CI, but for a different reason. The CI runs playbooks matching |
|
Is there anything that I can follow/check to know about this?
2018-05-14 18:15 GMT+02:00 pcahyna <notifications@github.com>:
… That's the point. It was deployed before being merged. I.e. the state of
the CI does not correspond to the GitHub sources. Confusing, huh?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#46 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAMEWB3ydeKufe5UsL1KVho33Q_wP-kTks5tya2ogaJpZM4T99gI>
.
--
Till Maas
Ansible RHEL Networking System Role Maintainer
Red Hat GmbH
|
|
It seems to me that the change for using Or is there another error? |
| @@ -7,7 +7,8 @@ | |||
| import socket | |||
| import itertools | |||
|
|
|||
| sys.path.insert(1, os.path.dirname(os.path.abspath(__file__))) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt this will work b/c in test_default.yml the module is imported from the same directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I see the tests are passing - the line was must have been unnecessary, given that Python puts the directory of the script in the path by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need the change to be able to run tox from the library/ directory for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't object against the change, I was just wondering why that line was ever there (and how removing it helps you with running anything, to me it seems like a no-op)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not removed but changed to the next line, which contains the correct value after the move. Not sure why it was there in the first place, though.
| - name: Test executing the role with default parameters | ||
| hosts: all | ||
| roles: | ||
| - linux-system-roles.network |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests seem to be gone, there are only deletions, no additions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, too much rebase fun. This should be fixed now.
| - name: Test executing the role with default parameters | ||
| hosts: all | ||
| roles: | ||
| - linux-system-roles.network |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests seem to be gone, there are deletions, no additions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, too much rebase fun. This should be fixed now.
|
Could you please squash the commits which rename files so that every file is renamed at most once in the commit series? Right now some of the files are renamed several times. |
|
I squashed the commits for the the new naming scheme and for travis/coveralls support. I kept the renaming of the |
Ansible Galaxy and https://fedoraproject.org/wiki/CI/Standard_Test_Interface suggest to use a /tests directory and a names matching test*.yml.
3064cc9 to
67e1f99
Compare
Do not run coveralls by default and do not run flake8 and pylint in travis. They make it always fail at the moment.
|
I rebased it. Also I updated the tox.ini to also show the coverage in the terminal. |
| linux-system-roles/network | ||
| ========================== | ||
| [](https://coveralls.io/github/linux-system-roles/network) | ||
| [](https://travis-ci.org/linux-system-roles/network) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to put the badges elsewhere because this does not look good when the source is used to generate user documentation in /usr/share/doc or similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would the other place be? All other projects I know show the badges there so that visitors can get a fast overview. AFAIK there is no other way to display them on the repo's main page. Checking /usr/share/doc, there are several packages that show the badges in README.md there, for example rpmlint, asciinema, criu, fwupd, neovim, pandoc, ... (checked on my Fedora 28 system).
Add initial support for coveralls