Skip to content
This repository was archived by the owner on Feb 4, 2019. It is now read-only.

Remove RestContext from Rackspace examples#35

Closed
jdaggett wants to merge 1 commit into
jclouds:masterfrom
rackerlabs:cloudfiles-updates
Closed

Remove RestContext from Rackspace examples#35
jdaggett wants to merge 1 commit into
jclouds:masterfrom
rackerlabs:cloudfiles-updates

Conversation

@jdaggett

Copy link
Copy Markdown

This PR removes RestContext from the Rackspace examples as the first step towards the work to be done in JCLOUDS-346.

Please refer to JCLOUDS-152 which discusses the removal of RestContext.

Each updated example was debugged/tested against the Rackspace Cloud IAD region.

The logo link has also been updated post site-branding... Thx!

@etoews

etoews commented Mar 13, 2014

Copy link
Copy Markdown
Member

Did you run the SmokeTest with these changes?

When that passes, I'm a +1.

@jdaggett

Copy link
Copy Markdown
Author

Yes! I ran SmokeTest against the code that was changed and everything passes.

@nacx

nacx commented Mar 20, 2014

Copy link
Copy Markdown
Contributor

Merged. Thanks!

@nacx nacx closed this Mar 20, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hm...do we recommend building two separate things here? Isn't that effectively creating two contexts, which is quite expensive? In this case, I'd say the old approach (building one context, then getting the compute service and unwrapping the context) is preferable?

@demobox

demobox commented Mar 20, 2014

Copy link
Copy Markdown
Member

Most of the cleanup is great, thanks @jdaggett! Just a comment about those examples where we use both the ComputeService view and the API. This refactoring now ends up creating two contexts in those cases, which I'm not sure is a good thing to be recommending?

@everett-toews @zack-shoylev What do you think..?

@jdaggett

Copy link
Copy Markdown
Author

@demobox Thanks for the feedback! Removal of RestContext was the first step in a bunch of example changes coming. I can update with your suggestions in the next PR which is underway now. WDYT?

@demobox

demobox commented Mar 20, 2014

Copy link
Copy Markdown
Member

I can update with your suggestions in the next PR which is underway now. WDYT?

Works for me! Would just be curious to see what we feel collectively about creating two contexts vs. using unwrap to get at the API...

@nacx

nacx commented Mar 20, 2014

Copy link
Copy Markdown
Contributor

Didn't notice that when going through the PR. and agree. Examples should be creating only one context!

@jdaggett

Copy link
Copy Markdown
Author

@demobox @nacx I agree that we should have a single context. Unfortunately, we can't remove RestContext<S, A> from Nova until the 1.8 timeframe, which is going to be some work. I will submit another PR to revert the changes to the Cloud Servers examples for the time being. Sound good?

cc @everett-toews

@demobox

demobox commented Mar 29, 2014

Copy link
Copy Markdown
Member

I will submit another PR to revert the changes to the Cloud Servers examples for the time being.
Sound good?

Sounds good! Thanks, @jdaggett!

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.

4 participants