-
Notifications
You must be signed in to change notification settings - Fork 204
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
Added Dangerfile to root directory of mapknitter #306
Conversation
Thanks for opening this pull request! |
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.
Hi @satyajeetmaharana, could you please make these little changes. They are required because we are using Dangerfile in mapknitter. Thanks!
Co-Authored-By: satyajeetmaharana <[email protected]>
Please check appropriate boxes in the checklist - #306 (comment) . Thanks! |
There's one more suggestion left. And, just a suggestion: Using Fixes #[issue-number] in the PR comment closes the tagged issue when PR gets merged so it's nice practice. Thanks! |
Hi @gauravano , Thank for the suggestion. I will keep it as a standard practice in my future PRs. For this issue, I have updated the PR comment. Hope it looks good now. |
Edited - #306 (comment) |
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.
There's still a suggestion pending!
Co-Authored-By: satyajeetmaharana <[email protected]>
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.
Looks good to me! Thanks @satyajeetmaharana for working on this.
Hey @jywarren, @satyajeetmaharana has added Dangerfile to the repo, could you please complete the installation of dangerbot. Thanks! |
Awesome. I think the remaining step is adjusting the https://danger.systems/guides/getting_started.html#setting-up-danger-to-run-on-your-ci |
See for example: https://github.com/publiclab/plots2/blob/master/.travis.yml -- i'll try this. |
OK, just noting that |
I see |
.travis.yml
Outdated
|
||
script: | ||
- docker-compose run web bash -c "rake test" | ||
- docker-compose run web bash -c "CI=TRUE GENERATE_REPORT=true rake test" |
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.
Hmm, oddly plots2
uses exec
instead of run
here. But that didn't work. So trying run
as it was originally here.
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.
ERROR: No container found for web_1
was the error...
Also adding the correct token to Travis for Danger posting! |
Sorry just going to start it again to have it see the token! |
Generated by 🚫 Danger |
Hooray! Looks like it's running, but just not properly parsing the errors produced in JUnit format. |
K interesting, it's saying:
|
I dunno, i don't see anything obvious so i'm going to restart it again to see if it was some kind of connectivity issue or an upstream server not responding properly? |
Aha!
Ok, looking... |
Looked carefully through plots2 and found we needed additions to test/test_helper.rb! |
Trying exec again... |
@jywarren Everytime you use 'run' either with docker or docker-compose it creates a new container, so anything you have done previously won't have desired behavior since you've done it in another container. I suggest changing everywhere that has 'run' to 'exec'. |
@@ -42,6 +42,9 @@ group :test do | |||
end | |||
|
|||
group :development do |
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 should be in a group :test
doesn't it? 🤔
hi @alaxalves - thank you! Would you like to try opening a PR based on this? You know a lot about Docker and we'd LOVE your help! |
Or merging this with your PR at #363 ? |
sounds good, thank you!
…On Fri, Mar 1, 2019 at 12:37 PM Álax de Carvalho Alves < ***@***.***> wrote:
Hey @jywarren <https://github.com/jywarren> I feel like it would be less
painful if we get #363 <#363>
before this. When its done I could open a PR regarding this. What do you
think? 😄
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#306 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/Ah3Vc2BRoO1OkaiOmOo6k5bpx66ftGeuks5vSWVQgaJpZM4aUXlX>
.
|
Hahaha, i got the notification from the plotsbot account and responded there, sorry, it thought i was plotsbot! 😂 |
@jywarren 😆 😆 😆 😆 Okay then!! |
Hi @alaxalves, as #363 is done. Can we push it to completion? Thank you! |
Code Climate has analyzed commit ca178b5 and detected 0 issues on this pull request. View more on Code Climate. |
Reopened in #926 |
Fixes #303
Make sure these boxes are checked before your pull request is ready to be reviewed and merged. Thanks!
rake test
Thanks!