-
Notifications
You must be signed in to change notification settings - Fork 0
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
OPENEUROPA-721: Update code-review and grump.yml.dist file. #23
Conversation
composer.json
Outdated
@@ -12,7 +12,7 @@ | |||
"phpunit/phpunit": "^5||^6.0" | |||
}, | |||
"require-dev": { | |||
"openeuropa/code-review": "^0.2", | |||
"openeuropa/code-review": "^0.3", |
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.
We don't need to state that we require 0.3 if we are not using the new features that are added in that version. We are still compatible with 0.2. A good idea would be to change the version range to ~0.2
then we are compatible with both 0.2, 0.3 and future versions.
I really like Drone but their error messages suck. I have no idea what this means. This could be either a problem in the I see that a line of whitespace was removed from the Drone file, maybe we can reinstate that? If that doesn't help I would go to the devops team for help. |
I believe in this case this case the drone error is caused because the drone.yml file is not present in the branch yet. |
OK this makes sense. I see that the master branch is also failing with the same error. It is normally against our acceptance procedure to allow PRs with failing tests, but this is not causing a regression and providing Drone support is out of scope for this PR. I think it is OK for me to accept this now. I have created an issue to take a look at Drone: #24 |
Why do we have Travis and Drone here as CI tool? |
Because the branch was created before drone support was added. I think it should be enough to just remove the travis file. |
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.
Fixed the merge by removing .travis.yml,
👍
Ready to be merged .
No description provided.