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

Ability to work with environments at chef > 0.10.0 #61

Merged
merged 1 commit into from Feb 20, 2013

Conversation

kulya
Copy link
Contributor

@kulya kulya commented Feb 4, 2013

@buildhive
Copy link

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

* @see ChefApi#environmentExists(String)
*/
@Named("environment:exists")
@GET // for a some reasons HEAD method is not supported
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method will perform the same API call than the one generated by the getEnvironment. If HEAD is not working, I'd just remove the environmentExists method.
However, did you test this with Hosted Chef? There are some API calls that use HEAD that work in the open source Chef and don't work in Hosted Chef. For those calls, we implement the standard behavior in the ChefAsyncAPI, and "patch" it in the concrete APIs where the method does not work. Take a look at the PatchedChefAsyncApi used in HostedChef and consider using the same approach.

If you don't have access to an open source Chef instance let me know and I'll test the HEAD method and let you know the results.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested at open source Chef and for a some reason HEAD method don't work for /environments/${environmentname} . I will remove this method

@nacx
Copy link
Member

nacx commented Feb 6, 2013

This looks good, but still needs a few more changes:

  • Try to remove all the warnings: remove unused imports, suppress the warnings for unused private members in domain objects, etc.
  • Add the jclouds headers in the classes you created.
  • Format the code according to the jclouds style: 3 spaces indentation and 120 characters line length.
  • Use the @SinceApiVersion annotation in the methods you added that are only available for a concrete version of Chef. This is specially important if those methods use parsers that will only be able to parse responses of a concrete version of Chef.
  • When developing a feature, consider creating a topic branch and make the pull request from it.

Nice work, btw!

@kulya
Copy link
Contributor Author

kulya commented Feb 6, 2013

I updated code based on your comments. I'm only not sure about @SinceApiVersion annotation usage. Can you please review it and if possible point me to usage example of this annotation. Also can i update pom, so it publish source jar as well?

@buildhive
Copy link

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

@buildhive
Copy link

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

@kulya
Copy link
Contributor Author

kulya commented Feb 12, 2013

Is there any other problems in my pull-request?

@nacx
Copy link
Member

nacx commented Feb 12, 2013

No, it looks good to me. We are just releasing 1.5.7 and 1.6.0-alpha.2 and I didn't want this changes to get into the alpha, to avoid introducing noise. Once we finish the release, I'll merge it, as it would be nice to have this for 1.6.0.

@kulya
Copy link
Contributor Author

kulya commented Feb 12, 2013

Ok, thanks a lot

@nacx
Copy link
Member

nacx commented Feb 19, 2013

Before merging this pull request, could you squash the commits into a single one?

Also, since this is your first contribution, you need to sign the individual contributor license agreement. Would you mind sending an email to adrian@jclouds.org so he can send you the agreement?

Change formating to jcloud standarts. Added SinceApiVersion annotation to new methods. Removed environmentExists method from api.

Added forgeted serializable name for attributes field of environment
@kulya
Copy link
Contributor Author

kulya commented Feb 19, 2013

Sent mail and squash commits into one.

@buildhive
Copy link

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

@nacx
Copy link
Member

nacx commented Feb 20, 2013

The idea of squashing the commits into a single one is to have the feature isolated into a single commit and to keep a clean commit tree once it is merged into the upstream.

If you take a look at the commits in this PR, there are 5 when it should be only one after squashing. Looking at the commits, it seems that the dca619b one is the commit you made grouping containing all the changes in the other commits, and the one that should be merged. Is this right?

If this is right, I would do the following to clean up this pull request and leave only that commit:

git fetch upstream
git reset --hard upstream/master   # This will discard all your changes, but don't worry
git cherry-pick dca619b        # This will add the good commit at the end of the commit tree
mvn clean install -P live ...  # Verify everything is still working
git push origin master -f      # You'll need to force push because we removed some commits

Once you push the changes, the PR should contain only the squashed commit.

(This is a nice tutorial about interactive rebasing and squashing).

@buildhive
Copy link

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

@kulya
Copy link
Contributor Author

kulya commented Feb 20, 2013

I have fix this problem. At least at https://github.com/kulya/jclouds-chef/commits/master i see only one commit.

codefromthecrypt pushed a commit that referenced this pull request Feb 20, 2013
Ability to work with environments at chef > 0.10.0
@codefromthecrypt codefromthecrypt merged commit bbbaecb into jclouds:master Feb 20, 2013
@nacx
Copy link
Member

nacx commented Feb 20, 2013

Thanks for the contribution @kulya!

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