-
Notifications
You must be signed in to change notification settings - Fork 19
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
Move to circleci #161
Move to circleci #161
Conversation
Code Climate has analyzed commit 32955cb and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 92.4% (0.0% change). View more on Code Climate. |
e807e4b
to
3b43a14
Compare
@dylanhitt would like to have your review on this. windows builds are capped, but I think it is acceptable given this is a small open source project 👍 |
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.
This looks good, I got to learn quite a bit from this 😄 . I'm not concerned about not being able to test macOS
. If we run into a problem with it later we can discuss some possible solutions.
You mentioned windows being capped? I can't really find that in their docs. If so how may windows builds do we get per X time frame?
commander-int-ssh-server | ||
|
||
docker build -t commander-int-test -f integration/containers/test/Dockerfile . | ||
docker run \ |
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.
you can always add docker run --rm
here. Avoiding some of the code below. But, you probably have a valid reason for not doing so
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.
👍 Just forgot it :D
Added --rm
, time shows how this container evolves. If it is used more often it definitely needs to change and cached.
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.
You can remove lines, 40 and 41 as well
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.
Right, done
I am not sure as well how much windows test capacity circleci provides. |
Actually looking into it more, it appears we're capped out completely. The 2500 credits are for both linux/windows. The windows jobs are taking a little under 200 credits per run... Maybe we can still test on windows circle does have the ability to only build on pull request. But then we wouldn't be building on the branch 🤷. Honestly, I'm fine with not running windows jobs except for master/tag. We could possibly add some type of branching exception if we need it for debugging like |
@dylanhitt looks better on the linux side:
Therefore we can execute windows build just on master & tags that we do not release broken versions for windows. |
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.
This is awesome, again let's see how it goes.
Fixes #160
./test.sh
or using its make targetmake test-coverage-all-dockerized
Checklist
Linux
,Windows
andmacOS
?