-
-
Notifications
You must be signed in to change notification settings - Fork 55
(#76) Implemented Container.restart() #80
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
Conversation
Job #80 is now in scope, role is |
Pull Request Test Coverage Report for Build 125
💛 - Coveralls |
1 similar comment
Pull Request Test Coverage Report for Build 125
💛 - Coveralls |
@amihaiemil/z everybody who has role |
This pull request #80 is assigned to @amihaiemil/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @amihaiemil/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer |
Manual assignment of issues is discouraged, see §19: -5 point(s) just awarded to @amihaiemil/z |
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.
@llorllale looks nice, just one comment
* @throws UnexpectedResponseException If the status response is not | ||
* expected. | ||
*/ | ||
void restart() throws IOException, UnexpectedResponseException; |
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.
@llorllale UnexpectedResponseException is runtime exception, no need to declare it :D
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.
@amihaiemil checkstyle fails if we don't include it.
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.
@llorllale strange, I don't remember checkstyle being that strict. Do you know how to add an exception for it quickly, or should we merge and I'll look into it later?
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.
@amihaiemil to be clear, checkstyle fails when I add the @throws
tag without declaring the exception. What you want is to include @throws
without declaring the exception in throws
, correct?
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.
@llorllale you're right, let's leave it this way then.
@amihaiemil/z this job was assigned to you 5days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please. |
@rultor merge it |
@amihaiemil OK, I'll try to merge now. You can check the progress of the merge here |
@amihaiemil @llorllale Oops, I failed. You can see the full log here (spent 22s)
|
Order was finished: +15 point(s) just awarded to @amihaiemil/z |
The job #80 is now out of scope |
This PR:
Note: I'm not sure if it's worth overloading
restart()
with atimeout
parameter (see the linked docs)