-
Notifications
You must be signed in to change notification settings - Fork 896
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
Enhance the the orchestrator to deal with more objects #15962
Enhance the the orchestrator to deal with more objects #15962
Conversation
28e80dc
to
89b77e4
Compare
I think this one is ready for review. |
yield(definition) if block_given? | ||
connection.create_deployment_config(definition) | ||
rescue KubeException => e | ||
raise unless e.message =~ /already exists/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check it if already exists rather than rescuing the error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Searching for a particular object that doesn't exist using the Kubeclient
gem raises the same exception so I figured it was easier to go this way.
def default_environment | ||
[ | ||
{:name => "GUID", :value => MiqServer.my_guid}, | ||
{:name => "DATABASE_SERVICE_NAME", :value => ENV["DATABASE_SERVICE_NAME"]}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, can you sort these alphabetically?
lib/container_orchestrator.rb
Outdated
Kubeclient::Client.new( | ||
uri, | ||
:auth_options => { :bearer_token_file => TOKEN_FILE }, | ||
:ssl_options => { :verify_ssl => OpenSSL::SSL::VERIFY_NONE } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have the certificate? If so, can we VERIFY_SSL
?
describe "#create_deployment_config" do | ||
it "creates a deployment config with the given name and edits" do | ||
expect(connection_stub).to receive(:create_deployment_config) do |definition| | ||
expect(definition[:metadata][:name]).to eq("test") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definition.fetch_path(:metadata, :name)
?
end | ||
|
||
subject.create_deployment_config("test") do |spec| | ||
spec[:spec][:template][:spec][:serviceAccount] = "test-account" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spec.store_path(:spec, :template, :spec, :serviceAccount, "test-account")
?
end | ||
end | ||
|
||
it "dosn't raise an exception for an existing object" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't
This puts the hash definitions of the API objects into definition methods which expose only a limited amount of customization. The new create / delete methods on the main ContainerOrchestrator object now yield the definition before issuing the API call to allow for further changes.
This will allow easy productization while still specifying an app name so that all of our objects can be referenced together.
89b77e4
to
0880821
Compare
Checked commits carbonin/manageiq@f62374c~...0880821 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
@bdunne You ok with this? |
This puts the hash definitions of the API objects into definition methods which expose only a limited amount of customization.
The new create / delete methods on the main ContainerOrchestrator object now yield the definition before issuing the API call to allow for further changes.
This is mostly related to the re-architecture, but won't hurt anything if we merge it now
¯\_(ツ)_/¯