Skip to content
This repository has been archived by the owner on Mar 25, 2018. It is now read-only.

base pr for - jclouds provider for google compute engine #1051

Merged
merged 1 commit into from
Dec 29, 2012

Conversation

dralves
Copy link
Contributor

@dralves dralves commented Dec 8, 2012

These are 3 small apis that are required by other apis in some place or another.

removed the other apis from the core api

removed the page system test

@dralves
Copy link
Contributor Author

dralves commented Dec 9, 2012

All comments were addressed. please review!

@dralves
Copy link
Contributor Author

dralves commented Dec 15, 2012

Addressed all reviewer comments and updated the downstream branches to reflect that (plus corrected potential similar issued).

Live tests pass, gist: https://gist.github.com/4292017

@jclouds, @adriancole, @mattstep please review so that I can start pushing the other apis!

Thanks

@cloudbees-pull-request-builder

jclouds-java-7-pull-requests #104 SUCCESS
This pull request looks good

@jclouds
Copy link
Collaborator

jclouds commented Dec 19, 2012

few things, but pretty much a go

cleanup:

  • do a Nullable/checkNotNull sweep as these things need to be consistent

get rid of extra types:

  • Metadata is an incidental type, only used for parsing it seems. It has no additional value above Map. as such, remove all uses of this as a public parameter or return type, and hide it in an internal package. only use it incidentally via gson.

make list stuff more coherent:

  • make sure you have list() methods on the apis to reduce clutter of passing empty objects
  • listPage and list are confusingly similar, not to mention the tight coupling of listOptions to both the resultset and also its use in list() which doesn't actually use a marker. the following should sort it:
    • remove marker from listOptions
    • remove references to listOptions in ListPage
    • rename methods named listPage() to listAtMarker() with 2 overloads.. listAtMarker(@nullable String marker) listAtMarker(@nullable String marker, ListOptions options).

correct docs wrt PEM:

  • apimetadata suggests that credential is a file. It should be a PEM literal as discussed earlier, in src/main, yet allow src/test to use a file. in other words, the conversion of file to pem literal should only take place in src/test or readme instructions on how to run the test. pem literal should be the only means supported in src/main even if javadoc notes how to read a pem (ex. Files.toString("/path/to/pem"...)

@dralves
Copy link
Contributor Author

dralves commented Dec 23, 2012

the only thing I left out is the readme stuff (although I did change the description of the credential to mention that it is a key a literal and not a file). The reason for this is that the readme will need an overall to add/mention the compute stuff anyway and I didn't want to loose the time doing a partial overall now.

@jclouds
Copy link
Collaborator

jclouds commented Dec 23, 2012

please sort out the defensive copy of collection, checkNotNull message strings, and make nullable fields Optional<X> and I will review again.

thanks for shining it up!

@dralves
Copy link
Contributor Author

dralves commented Dec 24, 2012

I think this should be the last set of changes. afaik everything is either optional or checked for null, every collection is defensibly copied.

thank you for reviewing! as soon as this goes in i'll make the other pr's asap (with changes consistent with these reviews). compute is nearly done (mostly implemented but still missing some tests) so it should be a short path to get gce completely in.

@jclouds
Copy link
Collaborator

jclouds commented Dec 24, 2012

okie hopefully last sweep done :) thanks for keeping at it, david

@cloudbees-pull-request-builder

jclouds-java-7-pull-requests #126 SUCCESS
This pull request looks good

@jclouds
Copy link
Collaborator

jclouds commented Dec 27, 2012

if you can, try to finish up the last sweep of comments today, as very soon, master will diverge from 1.5.x branch, and this will be more difficult to cherry-pick.

- addresses comments from jclouds#1010
- includes the following base apis
	- projects
	- operations
	- zones

These are 3 small apis that are required by other apis in some place or another.
@buildhive
Copy link

Adrian Cole » jclouds #729 SUCCESS
This pull request looks good
(what's this?)

@cloudbees-pull-request-builder

jclouds-java-7-pull-requests #144 SUCCESS
This pull request looks good

jclouds pushed a commit that referenced this pull request Dec 29, 2012
base pr for - jclouds provider for google compute engine
@jclouds jclouds merged commit ff1fb6c into jclouds:master Dec 29, 2012
@jclouds
Copy link
Collaborator

jclouds commented Dec 29, 2012

bam!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants