-
-
Notifications
You must be signed in to change notification settings - Fork 53
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
For #236: Added the rest of RtNetwork methods and fixed tests #245
Conversation
Job #245 is now in scope, role is |
Pull Request Test Coverage Report for Build 433
💛 - Coveralls |
Pull Request Test Coverage Report for Build 445
💛 - Coveralls |
This pull request #245 is assigned to @bkuzmic/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; there will be no monetary reward for this job |
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.
@paulodamaso Thanks for the PR. Please take a look at the comments
uri.build() | ||
); | ||
post.setEntity( | ||
new StringEntity( |
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.
@paulodamaso Please use JsonObject
here instead of composing it with String.format
. This will make the code reusable when adding more methods with optional parameters. For example, in the next step, new connect
method can be added to support optional EndpointConfig parameter, like this: connect(final String containerId, final JsonObject endpointConfig)
); | ||
post.setEntity( | ||
new StringEntity( | ||
String.format( |
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.
@paulodamaso See previous comment. Also use JsonObject
instead.
@bkuzmic Done, please take a look |
@rultor merge it |
@amihaiemil OK, I'll try to merge now. You can check the progress of the merge here |
@amihaiemil @paulodamaso Oops, I failed. You can see the full log here (spent 3min)
|
Job was finished in 27 hours, bonus for fast delivery is possible (see §36) |
Quality is low, no payment, see §31: |
The job #245 is now out of scope |
Job |
Payment to |
For #236:
RtNetwork
related too all other operations