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

add travis ci testing #85

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

add travis ci testing #85

wants to merge 7 commits into from

Conversation

msimerson
Copy link

@msimerson msimerson commented Sep 24, 2016

and lint tests with shellcheck

fixes #84

@msimerson
Copy link
Author

when you get travis set up, drop me a comment here and I'll push another commit to kick off a test.

@aqw
Copy link
Collaborator

aqw commented Sep 27, 2016

@msimerson Alrighty. I have signed up for Travis. If I had known it was a simple sign up form, I'd have done it right away. ;-)

---Alex

* and lint tests with shellcheck
@msimerson
Copy link
Author

Okay @aqw , this PR is complete and you can see the results of running the lint tests against all your files.

I've disabled a handful of shellcheck tests but you ought to review them and make sure my guesses about how you'd choose were accurate. There remains a handful of shellcheck lint errors that I think should be changed in the scripts, but I'll leave that to you to decide if you prefer to fix or suppress the warning.

Once those are addressed, then the Travis CI tests will pass and you'll get automated tests run on every PR.

@msimerson
Copy link
Author

msimerson commented Jan 6, 2017

I have updated this PR further:

  • I updated the code with a couple POSIX updates (see commit messages)
  • I disabled a few more shellcheck tests (see tests/lint.sh & inline # shellcheck disable=...

Now the test suite (including new lint tests) is automatically run and the build passes. This PR is now safe to merge.

However, I'd still recommend going through the list of disabled tests. In a couple cases, I think it would be better to change the code than disable the test, but for now, I'm erring on the side of "make fewer changes."

@msimerson
Copy link
Author

ping

@aqw
Copy link
Collaborator

aqw commented Jun 17, 2017

Hey @msimerson,

Thanks for the ping.

Quick mind-dump, as I'm in the middle of an all-consuming project at work that should be done in 4 weeks. After that, next on my list is zfsnap; cleaning things up, merging PRs, and finally, oh so finally, getting 2.0.0 released.

I've left a few comments on your commits, but my overall question is: can travis do what we need it to do? I have neither setup nor used travis before, so it's a question from ignorance rather than a counterargument.

More than anything, zfsnap needs to be tested across as many available shells on linux, FreeBSD, OS X, smartos, and Solaris. Different shells behave differently on different systems. The joys of portability...

The current vagrant system we're using is insecure and not easily used as CI.

At work, we are hoping to get a new server in 2 months; with that, I should be able to peel off off a few CPUs and use them for CI. Then we can have whatever OS targets we want.

So, the question for me is: how flexible is travis when it comes to integrating with private CI boxes?

---Alex

Copy link
Collaborator

@aqw aqw left a comment

Choose a reason for hiding this comment

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

This commit should be dropped. $() is not portable to all of the shells zfsnap supports.

---Alex

Copy link
Collaborator

@aqw aqw left a comment

Choose a reason for hiding this comment

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

I did not know about this syntax. How portable is it?

@msimerson
Copy link
Author

Your unknown preference is why that particular change was in it's own commit. So you could, if desired, use cherry-pick and only merge the others. I don't know how portable it is, but IIRC, it's part of the POSIX shell spec and bash for a very long time. Since you asked, here's some references for that change:

@aqw
Copy link
Collaborator

aqw commented Jun 18, 2017

@msimerson Ok. I'll be sure to drop that commit if/when merging.

I understand well why $() is superior to ``; I use it all the time. I can't remember which shell was problematic; it's in my notes somewhere; it was 3 years ago.

---Alex

note if cherry picking: just add SC2004 to the list of disabled checks in tests/lint.sh if you do not like this commit
@msimerson
Copy link
Author

I have removed the $() commit and added SC2006 to the exclude list.

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.

shorten code reviews with Travis and shellcheck
2 participants