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 to the repo #40

Merged
merged 2 commits into from
Dec 25, 2017
Merged

add Travis-CI to the repo #40

merged 2 commits into from
Dec 25, 2017

Conversation

zimbatm
Copy link
Member

@zimbatm zimbatm commented Dec 25, 2017

todo:

  • setup Travis-CI on the repo
  • test that it works

@lukateras
Copy link
Member

lukateras commented Dec 25, 2017

I don't think that would work because this test requires NixOS (for nixos-rebuild), and I don't think that Travis CI uses NixOS (they are likely to use something like Debian with Nix installed).

-I nixos-hardware-profile=$profile \
dry-build

if [ $? -ne 0 ]; then
Copy link
Member

Choose a reason for hiding this comment

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

Without this, test would never fail.

Copy link
Member Author

Choose a reason for hiding this comment

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

set -e takes care of this

Copy link
Member

Choose a reason for hiding this comment

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

Great! Didn't know about set -e.

@zimbatm
Copy link
Member Author

zimbatm commented Dec 25, 2017

fixed the test script so that it works on any system that has nix installed

@Mic92
Copy link
Member

Mic92 commented Dec 25, 2017

In doubt, it is possible to create VMs on non-nixos systems: nix-build '<nixpkgs/nixos>' -A vm -k -I nixos-config=./vm.nix

Copy link
Member

@lukateras lukateras left a comment

Choose a reason for hiding this comment

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

It works on Debian!

@zimbatm zimbatm force-pushed the travis-ci branch 5 times, most recently from 27d10c9 to b8f8d9a Compare December 25, 2017 16:51
@lukateras
Copy link
Member

lukateras commented Dec 25, 2017

I think shell scripts should be portable. Also, changing evaluation rules makes it harder to reason about. Let's leave it at #!/bin/sh, set -e, and find.

@lukateras
Copy link
Member

Sorry for force-pushing on top of your work, for reference, it's in fc032be7355ef8d1226fa77f0ef3a0c153e36100. Again, I think it makes the script more complex and I don't see any point in making it less verbose.

@lukateras lukateras merged commit 7dbceec into master Dec 25, 2017
@zimbatm
Copy link
Member Author

zimbatm commented Dec 25, 2017

@yegortimoshenko it's fine to disagree but please stop merging things into master while we are having a discussion. Anyways, follow-up in #44

@lukateras
Copy link
Member

lukateras commented Dec 25, 2017

@zimbatm I've only reverted your pull request to a previous state that we've agreed upon. The point of this pull request seemingly was to make Travis CI work and it works, so I've merged it.

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

3 participants