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

UI: Add Container Templates #10303

Merged
merged 1 commit into from
Oct 14, 2016
Merged

UI: Add Container Templates #10303

merged 1 commit into from
Oct 14, 2016

Conversation

zakiva
Copy link
Contributor

@zakiva zakiva commented Aug 7, 2016 •
edited by miq-bot
Loading

Display Container Templates in the UI. This PR is built on #10159 - persist Container Templates.

template1

temp_req

cc @simon3z @moolitayer

@zakiva zakiva changed the title UI: Add Container Templates [WIP] UI: Add Container Templates Aug 7, 2016
@zakiva
Copy link
Contributor Author

zakiva commented Aug 7, 2016

@miq-bot add_label providers/containers, enhancement, ui, wip

@miq-bot
Copy link
Member

miq-bot commented Aug 7, 2016

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

@miq-bot
Copy link
Member

miq-bot commented Aug 8, 2016

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

@zakiva
Copy link
Contributor Author

zakiva commented Aug 9, 2016

@zeari Please review

zeari
zeari reviewed Aug 15, 2016
View reviewed changes
app/helpers/application_helper/toolbar/container_template_center.rb
:confirm => N_("Perform SmartState Analysis on this item?")),
]
),
])
Copy link

@zeari zeari Aug 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@moolitayer SmartState Analysis Is relevant here?
Is this feature already implemented?(Would the button do what its supposed to)

Copy link

Choose a reason for hiding this comment

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

yeah this shouldn't be here

@zakiva zakiva force-pushed the add_template_ui branch 2 times, most recently from 298cc39 to 94a82ea Compare August 17, 2016 13:40
@miq-bot
Copy link
Member

miq-bot commented Aug 17, 2016

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

@miq-bot
Copy link
Member

miq-bot commented Aug 30, 2016

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

@simon3z
Copy link
Contributor

simon3z commented Sep 5, 2016

@zakiva @zeari I know that this depends on ManageIQ/kubeclient#185

Please let's review and get that merged ASAP so we can move ahead.

@miq-bot assign zakiva

@miq-bot miq-bot assigned zakiva and unassigned simon3z Sep 5, 2016
@miq-bot
Copy link
Member

miq-bot commented Sep 12, 2016

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

@zakiva zakiva force-pushed the add_template_ui branch 2 times, most recently from dd3f54a to 03a3f65 Compare September 18, 2016 10:03
@zakiva
Copy link
Contributor Author

zakiva commented Sep 18, 2016 •
edited
Loading

@simon3z @zeari I've added new tables for the template objects and parameters (screenshot above), please take another look.

@zakiva zakiva force-pushed the add_template_ui branch 2 times, most recently from 13faa66 to 8c8c69f Compare September 20, 2016 12:26
@zakiva zakiva force-pushed the add_template_ui branch 2 times, most recently from a920014 to 0e6784e Compare October 2, 2016 08:27
@simon3z
Copy link
Contributor

simon3z commented Oct 2, 2016

@dclarizio anyone from your side that could review this?

@miq-bot assign dclarizio

@miq-bot miq-bot assigned dclarizio and unassigned zakiva Oct 2, 2016
@simon3z
Copy link
Contributor

simon3z commented Oct 2, 2016

@zeari can you review/LGTM this?

zeari
zeari approved these changes Oct 2, 2016
View reviewed changes
Copy link

@zeari zeari left a comment

Choose a reason for hiding this comment

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

LGTM

@miq-bot
Copy link
Member

miq-bot commented Oct 3, 2016

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

@dclarizio
Copy link

@zakiva needs a rebase
@h-kataria @martinpovolny please review

@zeari
Copy link

zeari commented Oct 6, 2016

@miq-bot add_label euwe/yes

@miq-bot
Copy link
Member

miq-bot commented Oct 10, 2016

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

@simon3z
Copy link
Contributor

simon3z commented Oct 10, 2016

@zakiva @zeari can you rebase this?

@dclarizio
Copy link

@h-kataria please review code and tests. Thx, Dan

@miq-bot
Copy link
Member

miq-bot commented Oct 12, 2016

Checked commit zakiva@89780eb with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
20 files checked, 6 offenses detected

app/helpers/application_helper/toolbar/container_template_center.rb

  • ❗ - Line 16, Col 3 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.
  • ❗ - Line 3, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.

app/helpers/application_helper/toolbar/container_templates_center.rb

  • ❗ - Line 21, Col 3 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.
  • ❗ - Line 3, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.

app/views/configuration/_ui_2.html.haml

  • ⚠️ - Line 40 - Line is too long. [393/160]

app/views/layouts/listnav/_ems_container.html.haml

  • ⚠️ - Line 30 - Line is too long. [235/160]

@dclarizio dclarizio assigned h-kataria and unassigned dclarizio Oct 14, 2016
@dclarizio
Copy link

@h-kataria I'm ok with this, please review and merge when you are ok with it. Thx, Dan

h-kataria
h-kataria reviewed Oct 14, 2016
View reviewed changes
db/fixtures/miq_product_features.yml
:feature_type: admin
:identifier: container_template_admin
:children:
- :name: Remove
Copy link
Contributor

Choose a reason for hiding this comment

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

@zakiva why are we adding add/edit/remove features here and in routes file, i do not see these buttons in toolbar yml added in this PR. Toolbar file only has Edit tags button, so in my opinion redundant features and routes should be removed from this PR until we have support for those features.

@h-kataria
Copy link
Contributor

@zakiva i have opened #11941 to address comments in this PR. Other than the comment above this looks good for merging.

@h-kataria h-kataria added this to the Sprint 48 Ending Oct 24, 2016 milestone Oct 14, 2016
@h-kataria h-kataria merged commit b4eac1a into ManageIQ:master Oct 14, 2016
chessbyte pushed a commit that referenced this pull request Oct 14, 2016
@h-kataria @chessbyte
Merge pull request #10303 from zakiva/add_template_ui
3e18bb3 
UI: Add Container Templates
(cherry picked from commit b4eac1a)
@chessbyte
Copy link
Member

Euwe Backport details:

$ git log -1
commit 3e18bb36beb2b4af04cf3a9262e35b9f015df725
Author: Harpreet Kataria <hkataria@redhat.com>
Date:   Fri Oct 14 09:53:30 2016 -0400

    Merge pull request #10303 from zakiva/add_template_ui

    UI: Add Container Templates
    (cherry picked from commit b4eac1a5b1a2f663bad984d2b55014266d2765ca)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@h-kataria h-kataria h-kataria left review comments

@zeari zeari zeari approved these changes

Assignees

@h-kataria h-kataria

Labels
enhancement euwe/backported providers/containers ui
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@zakiva @miq-bot @simon3z @zeari @dclarizio @h-kataria @chessbyte @moolitayer