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

Linter cleanups #1383

Merged
merged 10 commits into from
Oct 29, 2017
Merged

Linter cleanups #1383

merged 10 commits into from
Oct 29, 2017

Conversation

pchaigno
Copy link
Contributor

@pchaigno pchaigno commented Oct 7, 2017

Would it be possible to enable Travis CI for the repo before merging this pull request?

@goldshtn
Copy link
Collaborator

goldshtn commented Oct 7, 2017

Paddin my ignorance, what would we gain by having Travis CI build this repository? We already have three Jenkins build bots.

@goldshtn
Copy link
Collaborator

goldshtn commented Oct 7, 2017

Paddin = pardon, silly phone :)

@pchaigno
Copy link
Contributor Author

pchaigno commented Oct 7, 2017

I'm 👍 with having this as part of the Jenkins' pipeline.

I initially went for Travis CI (proposed in #920, added in #987) because, contrary to the other tests, lint checks should not depend on the underlying OS version and we thus don't need to start a VM; a Travis container is faster. I also really appreciate the integration with GitHub. In particular, the configuration is part of the source code, which doesn't seem to be the case for the current Jenkins build bots.

@goldshtn
Copy link
Collaborator

/cc @drzaeus77 I have good experience with TravisCI, just a bit worried about the duplication. If it's only going to be used as a linter, I think it doesn't hurt -- it does provide very fast build feedback unlike the current build bots, and requires no additional maintenance on our part.

@4ast
Copy link
Member

4ast commented Oct 26, 2017

LGTM. Merge ready? cc @drzaeus77

@4ast 4ast merged commit ee433d4 into iovisor:master Oct 29, 2017
@pchaigno pchaigno deleted the pep8 branch October 29, 2017 07:50
@pchaigno
Copy link
Contributor Author

Without Travis-CI (or some other CI) enabled, we'll probably have to do this again in a few weeks...

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.

3 participants