Skip to content
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

Add abstract orchestration modeling #45

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@chrisroberts
Copy link

chrisroberts commented Jun 2, 2014

Looking for feedback

begin
require "fog/#{provider}/network"
rescue LoadError
# is there a reason for this being automatic?

This comment has been minimized.

@krames

krames Jun 3, 2014

Member

That is a very good question!

I dug around in an older version of fog before fog-core was split out and found this commit.

Glancing through the code, I don't see a reason why you would want networking. My guess is that this was a copy/paste fail, but let's leave this in here for now and we can create a separate PR at a later date to remove this.

This comment has been minimized.

@chrisroberts
def find_by_id(id)
self.find {|event| event.id == id}
end
alias_method :get, :find_by_id

This comment has been minimized.

@krames

krames Jun 3, 2014

Member

Can I get you to just call this method get? We are trying to enforce this convention for every collection to help encourage developers to write portable code.

self.find {|output| output.key == key}
end
alias_method :find_by_id, :find_by_key
alias_method :get, :find_by_key

This comment has been minimized.

@krames

krames Jun 3, 2014

Member

Can I get you to just call this method get for the reasons above.

def find_by_physical_id(id)
self.find {|resource| resource.physical_resource_id == id}
end
alias_method :find_by_id, :find_by_physical_id

This comment has been minimized.

@krames

krames Jun 3, 2014

Member

I can definitely see an argument for deviating from get here for find_by_physical_id and find_by_logical_id. Can I get you to remove the find_by_id though?

def find_by_id(id)
self.find {|stack| stack.id == id}
end
alias_method :get, :find_by_id

This comment has been minimized.

@krames

krames Jun 3, 2014

Member

Can I get you to just call this method get for the reasons above.

@chrisroberts

This comment has been minimized.

Copy link
Author

chrisroberts commented Jun 3, 2014

@krames No problem renaming the methods over to #get. Do you want the #find_by_ids to still hang around as aliases, or just be removed completely? (fwiw, I'm completely indifferent)

@krames

This comment has been minimized.

Copy link
Member

krames commented Jun 3, 2014

@chrisroberts I would say remove them so we can encourage people to use get.

@chrisroberts

This comment has been minimized.

Copy link
Author

chrisroberts commented Jun 3, 2014

@krames cool, already removed. aside from throwing in some docs on the models, anything else missing or questionable in this PR?

@krames

This comment has been minimized.

Copy link
Member

krames commented Jun 4, 2014

@chrisroberts Thanks! I just want to loop the @geemus and @tokengeek in the DSL you added (events, resources, and outputs). We are starting to think about refining some of our abstractions and I think this PR would be a good point of discussion.

raise NotImplemented
end
end

This comment has been minimized.

@krames

krames Jun 4, 2014

Member

@geemus @tokengeek This PR has added a DSL to help facilitate adding a common abstraction for providers while keeping the high level logic at more generic level. You can see the concrete implementation in fog/fog#2971.

What are your thoughts on this approach?

@geemus

This comment has been minimized.

Copy link
Member

geemus commented Jun 5, 2014

Looks pretty good, at least at a high level (I don't feel I have sufficient concrete experience with orchestration to be able to comment on the specifics).

@krames

This comment has been minimized.

Copy link
Member

krames commented Jun 10, 2014

@tokengeek @geemus I am planning on merging this PR in by the end of the week. In the meantime, @tokengeek let me know if you have reservations about this approach.

@tokengeek

This comment has been minimized.

Copy link
Member

tokengeek commented Jun 10, 2014

@krames - Sorry not really had time. I also haven't looked at orchestration in detail.

If anyone else in the community with experience can have a look that would be useful.

@krames

This comment has been minimized.

Copy link
Member

krames commented Jun 10, 2014

@geemus @tokengeek I was less interested in running orchestration past you than I was the DSL (resources, events, outputs) that @chrisroberts introduced which would allow us to move more logic into fog-core rather than the provider itself. Do you guys like this pattern? Should give it a shot with orchestration?

https://github.com/chrisroberts/fog-core/blob/feature/orchestration/lib/fog/orchestration/models/stack.rb#L10-L29

@geemus

This comment has been minimized.

Copy link
Member

geemus commented Jun 10, 2014

@krames hmm. What does that look like in usage then?

@chrisroberts

This comment has been minimized.

Copy link
Author

chrisroberts commented Jun 10, 2014

@geemus this PR on fog (fog/fog#2971) is dependent on this PR and is the concrete implementation on top of this abstraction.

@geemus

This comment has been minimized.

Copy link
Member

geemus commented Jun 10, 2014

@krames so, you are pointing to the possibility of using the same pattern (at least in theory) for say directory.files?

@krames

This comment has been minimized.

Copy link
Member

krames commented Jun 10, 2014

Potentially.

@geemus

This comment has been minimized.

Copy link
Member

geemus commented Jun 10, 2014

It is more meta, which can potentially be hard to debug, but it would simplify the code and reduce some duplication (especially in compute/storage where there are a lot of these repeated all over the different providers). I think there is certainly some merit to something like this anyway.

@chrisroberts

This comment has been minimized.

Copy link
Author

chrisroberts commented Jul 11, 2014

Unless there are any other opinions or concerns, I'm going to finish out both PRs and throw some documentation on them too. Thanks!

@geemus

This comment has been minimized.

Copy link
Member

geemus commented Jul 14, 2014

@chrisroberts sounds like a plan, thanks!

@chrisroberts

This comment has been minimized.

Copy link
Author

chrisroberts commented Aug 6, 2014

@geemus filled out docs. was going to start on defining abstract style specs for concrete implementations to subclass and work against, but i think i may need some guidance on overall approach and it may be better suited for a different PR?

Also wrapped up the fog PR that relies on this changeset: fog/fog#2971

@geemus

This comment has been minimized.

Copy link
Member

geemus commented Aug 7, 2014

@chrisroberts thanks for the work, seems awesome, sorry I haven't quite gotten to it yet. I think I should be able to give it a more thorough review tomorrow.

@geemus

This comment has been minimized.

Copy link
Member

geemus commented Aug 11, 2014

Adding abstract specs would be great and definitely could help keep things on track as further providers come in to this space. Just let me know as you have questions, keeping it as a distinct PR seems like a good plan.

@geemus

This comment has been minimized.

Copy link
Member

geemus commented Aug 11, 2014

Looks pretty good, though I must admit I lack some experience in the orchestration side in particular. I think a great next step might be to write up some docs describing at a high level what usage looks like, kind of like these examples: compute, storage. I think that would help me to understand how the pieces fit together a bit better (as well as help highlight gaps in the design, if any). What do you think? Thanks!

@chrisroberts

This comment has been minimized.

Copy link
Author

chrisroberts commented Aug 11, 2014

@geemus I have seen some discussions here and there about providing an abstract spec. Are there any resources that fill this out more, or should I just take a shot and we can discuss the result (will cut a new PR for it)?

I'll start on a high level description doc. Will link PR in here once I have it going.

Thanks!

@geemus

This comment has been minimized.

Copy link
Member

geemus commented Aug 11, 2014

@chrisroberts the example would be like: https://github.com/fog/fog/tree/master/tests/compute. Hope that clarifies at least a bit. We should move those in to fog-core probably, but that is where they still live at present.

@chrisroberts

This comment has been minimized.

Copy link
Author

chrisroberts commented Aug 12, 2014

@geemus I ran across discussions about moving from shindo to minitest. Is that still an active goal? I'm indifferent to the library, would just like to target the correct one so it won't need to be re-implemented at a later date.

@chrisroberts

This comment has been minimized.

Copy link
Author

chrisroberts commented Aug 12, 2014

@geemus PR for high level overview: fog/fog.github.com#28

@geemus

This comment has been minimized.

Copy link
Member

geemus commented Aug 12, 2014

@chrisroberts yeah, moving off shindo is definitely the plan. We ended up landing at minitest (though I still think about rspec sometimes), so using minitest would be preferred here. Just let me know if you have further questions from there. Thanks!

@kbrock

This comment has been minimized.

Copy link

kbrock commented Oct 9, 2014

This looks like great stuff. What is needed to push this through?
I can fork and clean up any concerns if you need.

@kbrock

This comment has been minimized.

Copy link

kbrock commented Nov 6, 2014

Anything we can do to help push this forward?

@geemus

This comment has been minimized.

Copy link
Member

geemus commented Nov 11, 2014

@kbrock I think high level docs and some testing around the abstraction itself would be great. Basically my concern at this point is around how hard (or not) it will be for us to adapt existing stuff to fit this pattern. Docs and tests both help to get us there (and ensure we don't drift). If you'd like to help with one or both that would be great. Just let me know if there is anything I can clarify or help with. Thanks!

@chrisroberts

This comment has been minimized.

Copy link
Author

chrisroberts commented Nov 19, 2014

@kbrock i've moved focus on the orchestration stuff from this branch to a new lib. There are basic usage docs in the fog site repo. I hadn't focused much on the openstack part, but it should be mostly interchangeable with the RS implementation (and has been within spinoff lib). Please feel free to punt this forward if you feel like doing so. Cheers!

@bzwei

This comment has been minimized.

Copy link

bzwei commented Jan 12, 2015

What's the fate of this PR? I need a full stack of OpenStack Orchestration features. I saw the discussions in fog/fog#2971 where stated RackSpace Orchestration had been merged but not based on the modeling from this PR. If the modeling here is still the plan, I would like to help to push it through and then refactor OpenStack orchestration.

@plribeiro3000

This comment has been minimized.

Copy link
Member

plribeiro3000 commented Apr 2, 2018

The latest commit here has almost 4 years. I'm closing this due l believe there wont be any more development on this.

Reopen or comment back if thats not the case.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.