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.

Sign up for GitHub

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

Jump to bottom

Addition of OpenShift provider #2611

Merged
merged 1 commit into from
May 14, 2015
Merged

Addition of OpenShift provider #2611

merged 1 commit into from
May 14, 2015

Conversation

abonas
Copy link
Member

@abonas abonas commented Apr 14, 2015

Contains provider (with a mixin to avoid inheritance from k8s provider), parser, refresher,
refresh worker and persistence of container_project and container_route

@abonas
Copy link
Member Author

abonas commented Apr 14, 2015

@Fryguy , @blomquisg the provider is missing a second call to kubeclient to get k8s inventory.
Advice is appreciated how to add that part, since it's not that common in other providers to collect inventory from more than one source.

@chessbyte chessbyte added the wip label Apr 14, 2015
@abonas
Copy link
Member Author

abonas commented Apr 14, 2015

@miq-bot add_label providers/containers

@miq-bot
Copy link
Member

miq-bot commented Apr 14, 2015

<gemfile_checker />@JPrause @jvlcek Gemfile changes detected in commit abonas@8629ef8. Please review.

@miq-bot
Copy link
Member

miq-bot commented Apr 14, 2015

<gemfile_checker />@JPrause @jvlcek Gemfile changes detected in commit abonas@60ca9d7. Please review.

1 similar comment
@miq-bot
Copy link
Member

miq-bot commented Apr 14, 2015

<gemfile_checker />@JPrause @jvlcek Gemfile changes detected in commit abonas@60ca9d7. Please review.

@miq-bot
Copy link
Member

miq-bot commented Apr 14, 2015

<gemfile_checker />@JPrause @jvlcek Gemfile changes detected in commit abonas@9f955eb. Please review.

@miq-bot
Copy link
Member

miq-bot commented Apr 14, 2015

<gemfile_checker />@JPrause @jvlcek Gemfile changes detected in commit abonas@2ab197e. Please review.

@miq-bot
Copy link
Member

miq-bot commented Apr 14, 2015

<gemfile_checker />@JPrause @jvlcek Gemfile changes detected in commit abonas@e318a67. Please review.

Fryguy
Fryguy reviewed Apr 14, 2015
View reviewed changes
vmdb/app/helpers/ems_container_helper/textual_summary.rb
@@ -10,6 +10,9 @@ def textual_group_properties

def textual_group_relationships
items = %w(container_nodes container_services container_groups)
if @ems.class.ems_type.eql? 'openshift'
Copy link
Member

Choose a reason for hiding this comment

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

Prefer == over .eql? However, even so, this should just be @ems.kind_of?(EmsOpenshift)

Copy link
Member Author

Choose a reason for hiding this comment

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

refer == over .eql? However, even so, this should just be @ems.kind_of?(EmsOpenshift)

sure, will do "kind_of"

@miq-bot
Copy link
Member

miq-bot commented Apr 24, 2015

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@miq-bot
Copy link
Member

miq-bot commented Apr 26, 2015

<gemfile_checker />@JPrause @jvlcek Gemfile changes detected in commit abonas@5fe910c. Please review.

@abonas
Copy link
Member Author

abonas commented Apr 27, 2015

@Fryguy / @blomquisg can this be merged please?
It delays a lot of other dependent patches and it also must be rebased practically every day.

jrafanie
jrafanie reviewed May 5, 2015
View reviewed changes
vmdb/app/controllers/ems_common.rb
@@ -702,6 +702,10 @@ def get_form_vars
@edit[:new][:emstype] = params[:server_emstype]
if ["openstack", "openstack_infra"].include?(params[:server_emstype])
@edit[:new][:port] = @ems.port ? @ems.port : 5000
elsif params[:server_emstype] == EmsKubernetes.ems_type
@edit[:new][:port] = @ems.port ? @ems.port : EmsKubernetes.new.port
Copy link
Member

Choose a reason for hiding this comment

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

@abonas I think this should be a constant, EmsKubernetes::DEFAULT_PORT and have dafault_value_for use that constant. I don't see the need to instantiate an object here.

This can be fixed in a followup PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

@abonas I think this should be a constant, EmsKubernetes::DEFAULT_PORT and have dafault_value_for use that constant. I don't see the need to instantiate an object here.

This can be fixed in a followup PR.

@jrafanie , ack. lets get this merged and I'll fix it separately. (I filed an issue so it won't be lost)

@jrafanie
Copy link
Member

jrafanie commented May 5, 2015

LGTM.

@jrafanie
Copy link
Member

jrafanie commented May 5, 2015

cc @Fryguy, please do final review

@abonas abonas mentioned this pull request May 6, 2015
Closed
@miq-bot
Copy link
Member

miq-bot commented May 7, 2015

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@miq-bot
Copy link
Member

miq-bot commented May 11, 2015

<gemfile_checker />@JPrause @jvlcek Gemfile changes detected in commit abonas@35309e3. Please review.

@miq-bot
Copy link
Member

miq-bot commented May 11, 2015

<gemfile_checker />@JPrause @jvlcek Gemfile changes detected in commit abonas@cb5418a. Please review.

@miq-bot
Copy link
Member

miq-bot commented May 11, 2015

<gemfile_checker />@JPrause @jvlcek Gemfile changes detected in commit abonas@8c78bb9. Please review.

@miq-bot
Copy link
Member

miq-bot commented May 11, 2015

<gemfile_checker />@JPrause @jvlcek Gemfile changes detected in commit abonas@9c0cd6b. Please review.

@abonas
Copy link
Member Author

abonas commented May 11, 2015

it needs to be rebased against #2891
and then add the additional openshift class to automation.

@abonas
Addition of OpenShift provider
506e369 
This change contains:
provider, parser, refresher,
refresh worker, persistence of container_project and container_route
and autofill of default port values for both containers providers in the UI
It also contains the relevant automation class.
@abonas
Copy link
Member Author

abonas commented May 12, 2015

rebased and contains the additional openshift class in automation.

@miq-bot
Copy link
Member

miq-bot commented May 12, 2015

<gemfile_checker />@JPrause @jvlcek Gemfile changes detected in commit abonas@506e369. Please review.

@miq-bot
Copy link
Member

miq-bot commented May 12, 2015

Checked commit abonas@506e369 with rubocop 0.27.1
19 files checked, 0 offenses detected
Everything looks good. ⭐

@abonas
Copy link
Member Author

abonas commented May 12, 2015

@Fryguy , this is all yours :)
(is travis stuck again?)

@jrafanie
Copy link
Member

Restarted @abonas

@abonas
Copy link
Member Author

abonas commented May 13, 2015

@Fryguy @blomquisg @jrafanie anything prevents this from being merged?

Fryguy added a commit that referenced this pull request May 14, 2015
@Fryguy
Merge pull request #2611 from abonas/openshiftprovidermixin
05798aa 
Addition of OpenShift provider
@Fryguy Fryguy merged commit 05798aa into ManageIQ:master May 14, 2015
@Fryguy Fryguy added this to the Sprint 24 Ending June 1, 2015 milestone May 14, 2015
@Fryguy Fryguy mentioned this pull request May 14, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement providers/containers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@abonas @miq-bot @Fryguy @jrafanie @simon3z @chessbyte