You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jclouds.apache.org by e-do <no...@github.com> on 2013/08/24 20:57:04 UTC
[jclouds] JCLOUDS-241: Added missing options to
ListUsageRecordsOptions (#124)
Added missing annotations in UsageRecord.java.
You can merge this Pull Request by running:
git pull https://github.com/e-do/jclouds master
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/124
-- Commit Summary --
* Added type, page, and pagesize to ListUsageRecordsOptions and
* Since id can be null, the annotation @Nullable should be used.
-- File Changes --
M apis/cloudstack/src/main/java/org/jclouds/cloudstack/domain/UsageRecord.java (5)
M apis/cloudstack/src/main/java/org/jclouds/cloudstack/options/ListUsageRecordsOptions.java (32)
M apis/cloudstack/src/test/java/org/jclouds/cloudstack/options/ListUsageRecordsOptionsTest.java (19)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/124.patch
https://github.com/jclouds/jclouds/pull/124.diff
Re: [jclouds] JCLOUDS-241: Added missing options to
ListUsageRecordsOptions (#124)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #168](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/168/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/124#issuecomment-23215320
Re: [jclouds] JCLOUDS-241: Added missing options to
ListUsageRecordsOptions (#124)
Posted by BuildHive <no...@github.com>.
[jclouds ยป jclouds #338](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/338/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/124#issuecomment-23215152
Re: [jclouds] JCLOUDS-241: Added missing options to
ListUsageRecordsOptions (#124)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #628](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/628/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/124#issuecomment-23215275
Re: [jclouds] JCLOUDS-241: Added missing options to
ListUsageRecordsOptions (#124)
Posted by Ignasi Barrera <no...@github.com>.
>@nacx: I think we were still unsure as to whether we need to verify somewhere that the query replacement stuff works as expected - in a dedicated test, as part of another test, wherever.
Or we decide that we don't need to test this after all.
Ok! Let's wait for the tests then before merging this.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/124#issuecomment-44246229
Re: [jclouds] JCLOUDS-241: Added missing options to
ListUsageRecordsOptions (#124)
Posted by Andrew Phillips <no...@github.com>.
> @@ -367,6 +367,7 @@ protected UsageRecord(String id, @Nullable String description, @Nullable String
> this.usageType = usageType;
> }
>
> + @Nullable
> public String getId() {
What I usually do is to create another branch based on this one, then rebase both branches on master and keep on the commits relevant to each issue. More details here:
http://git-scm.com/book/en/Git-Branching-Rebasing
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/124/files#r6371187
Re: [jclouds] JCLOUDS-241: Added missing options to
ListUsageRecordsOptions (#124)
Posted by Andrew Phillips <no...@github.com>.
> @@ -75,4 +78,20 @@ public void testKeywordStatic() {
> ListUsageRecordsOptions options = keyword("bob");
> assertEquals(ImmutableSet.of("bob"), options.buildQueryParameters().get("keyword"));
> }
> +
> + public void testTypeStatic() {
> + ListUsageRecordsOptions options = type("3");
> + assertEquals(ImmutableSet.of("3"), options.buildQueryParameters().get("type"));
> + }
> +
> + public void testPageStatic() {
> + ListUsageRecordsOptions options = page("1");
> + assertEquals(ImmutableSet.of("1"), options.buildQueryParameters().get("page"));
> + }
> +
> + public void testPageSizeStatic() {
> + ListUsageRecordsOptions options = pageSize("500");
> + assertEquals(ImmutableSet.of("500"), options.buildQueryParameters().get("pagesize"));
> + }
The options stuff also looks OK, but are they supposed to be used anywhere, e.g. in building request? Or is that all happening automagically? Should we be extending an *ExpectTest to see how these get added to an HTTP request?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/124/files#r5966700
Re: [jclouds] JCLOUDS-241: Added missing options to
ListUsageRecordsOptions (#124)
Posted by Everett Toews <no...@github.com>.
During release week we like to do a little house cleaning in the jclouds world. That means sweeping out the pull request queue.
This PR is over 6 months old. Please update us on its status here. If we don't hear anything, we will take that as lazy consensus that the PR is no longer relevant and it will be closed on Friday, May 30.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/124#issuecomment-44344993
Re: [jclouds] JCLOUDS-241: Added missing options to
ListUsageRecordsOptions (#124)
Posted by e-do <no...@github.com>.
> @@ -367,6 +367,7 @@ protected UsageRecord(String id, @Nullable String description, @Nullable String
> this.usageType = usageType;
> }
>
> + @Nullable
> public String getId() {
Sorry for the late response. What's the best way in separating a single pull request into two pull requests?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/124/files#r6368708
Re: [jclouds] JCLOUDS-241: Added missing options to
ListUsageRecordsOptions (#124)
Posted by Andrew Phillips <no...@github.com>.
@e-do: Do you have any update on the review comments? Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/124#issuecomment-24473931
Re: [jclouds] JCLOUDS-241: Added missing options to
ListUsageRecordsOptions (#124)
Posted by Andrew Phillips <no...@github.com>.
> @@ -367,6 +367,7 @@ protected UsageRecord(String id, @Nullable String description, @Nullable String
> this.usageType = usageType;
> }
>
> + @Nullable
> public String getId() {
These changes look fine. Could you split these off into a separate PR? They don't seem to be related to the changes below.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/124/files#r5966693
Re: [jclouds] JCLOUDS-241: Added missing options to
ListUsageRecordsOptions (#124)
Posted by Andrew Phillips <no...@github.com>.
> @@ -75,4 +78,20 @@ public void testKeywordStatic() {
> ListUsageRecordsOptions options = keyword("bob");
> assertEquals(ImmutableSet.of("bob"), options.buildQueryParameters().get("keyword"));
> }
> +
> + public void testTypeStatic() {
> + ListUsageRecordsOptions options = type("3");
> + assertEquals(ImmutableSet.of("3"), options.buildQueryParameters().get("type"));
> + }
> +
> + public void testPageStatic() {
> + ListUsageRecordsOptions options = page("1");
> + assertEquals(ImmutableSet.of("1"), options.buildQueryParameters().get("page"));
> + }
> +
> + public void testPageSizeStatic() {
> + ListUsageRecordsOptions options = pageSize("500");
> + assertEquals(ImmutableSet.of("500"), options.buildQueryParameters().get("pagesize"));
> + }
> The TestNG assertions are backwards here :+1:
Could catch, @jdaggett - thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/124/files#r13092065
Re: [jclouds] JCLOUDS-241: Added missing options to
ListUsageRecordsOptions (#124)
Posted by Ignasi Barrera <no...@github.com>.
How should be proceed with this PR? Is still work to be done? Or should we close it?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/124#issuecomment-34921284
Re: [jclouds] JCLOUDS-241: Added missing options to
ListUsageRecordsOptions (#124)
Posted by Jeremy Daggett <no...@github.com>.
> @@ -75,4 +78,20 @@ public void testKeywordStatic() {
> ListUsageRecordsOptions options = keyword("bob");
> assertEquals(ImmutableSet.of("bob"), options.buildQueryParameters().get("keyword"));
> }
> +
> + public void testTypeStatic() {
> + ListUsageRecordsOptions options = type("3");
> + assertEquals(ImmutableSet.of("3"), options.buildQueryParameters().get("type"));
> + }
> +
> + public void testPageStatic() {
> + ListUsageRecordsOptions options = page("1");
> + assertEquals(ImmutableSet.of("1"), options.buildQueryParameters().get("page"));
> + }
> +
> + public void testPageSizeStatic() {
> + ListUsageRecordsOptions options = pageSize("500");
> + assertEquals(ImmutableSet.of("500"), options.buildQueryParameters().get("pagesize"));
> + }
@e-do @demobox The TestNG assertions are backwards here :
`assertEquals(actual, expected)` instead of `assertEquals(expected, actual)`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/124/files#r13088706
Re: [jclouds] JCLOUDS-241: Added missing options to
ListUsageRecordsOptions (#124)
Posted by e-do <no...@github.com>.
> @@ -75,4 +78,20 @@ public void testKeywordStatic() {
> ListUsageRecordsOptions options = keyword("bob");
> assertEquals(ImmutableSet.of("bob"), options.buildQueryParameters().get("keyword"));
> }
> +
> + public void testTypeStatic() {
> + ListUsageRecordsOptions options = type("3");
> + assertEquals(ImmutableSet.of("3"), options.buildQueryParameters().get("type"));
> + }
> +
> + public void testPageStatic() {
> + ListUsageRecordsOptions options = page("1");
> + assertEquals(ImmutableSet.of("1"), options.buildQueryParameters().get("page"));
> + }
> +
> + public void testPageSizeStatic() {
> + ListUsageRecordsOptions options = pageSize("500");
> + assertEquals(ImmutableSet.of("500"), options.buildQueryParameters().get("pagesize"));
> + }
Sorry for the late response.
Yes, these options / parameters are used when providing options in the request message. Here's a link to the API: http://cloudstack.apache.org/docs/api/apidocs-4.0.0/root_admin/listUsageRecords.html.
These test were added similar to the existing options - account Id, domain Id, keyword, etc. I believe the purpose of these tests is to make sure that the request options / parameters are being set correctly.
I looked and I couldn't find an ExpectTest that this might fit under. Perhaps we can add those options to one of the existing tests in GlobalUsageApiTest.java?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/124/files#r6368831
Re: [jclouds] JCLOUDS-241: Added missing options to
ListUsageRecordsOptions (#124)
Posted by Andrew Phillips <no...@github.com>.
> Just a reminder. What's missing to have this ready to merge?
@nacx: I think we were still unsure as to whether we need to verify _somewhere_ that the query replacement stuff works as expected - in a dedicated test, as part of another test, wherever.
Or we decide that we don't need to test this after all.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/124#issuecomment-44230307
Re: [jclouds] JCLOUDS-241: Added missing options to
ListUsageRecordsOptions (#124)
Posted by e-do <no...@github.com>.
Sorry for the really late responses. I missed this. Is there a way to create an reminder email within github?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/124#issuecomment-24484378
Re: [jclouds] JCLOUDS-241: Added missing options to
ListUsageRecordsOptions (#124)
Posted by Everett Toews <no...@github.com>.
Closed #124.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/124#event-126633666
Re: [jclouds] JCLOUDS-241: Added missing options to
ListUsageRecordsOptions (#124)
Posted by Ignasi Barrera <no...@github.com>.
Just a reminder. What's missing to have this ready to merge?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/124#issuecomment-43721635