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

Adding default labels 'com.redhat.component' to chargback assignments #3144

Merged
merged 1 commit into from
Jan 8, 2018

Conversation

nimrodshn
Copy link
Contributor

@nimrodshn nimrodshn commented Dec 28, 2017

What This Does
Makes the label com.redhat.component as default label for docker container images in chargback assignments screen.

Why
This makes it easier for customers to leverage RH product labels, instead of scanning through what could be large number of image labels.

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1417236
GIF:
default_labels

cc: @himdel , @zeari

@nimrodshn nimrodshn changed the title Adding default labels Adding default labels 'com.redhat.component' to chargback assignments Dec 28, 2017
@nimrodshn nimrodshn changed the title Adding default labels 'com.redhat.component' to chargback assignments [WIP] Adding default labels 'com.redhat.component' to chargback assignments Dec 28, 2017
@nimrodshn
Copy link
Contributor Author

@zeari not really sure what to put in the popover, any suggestions?

@nimrodshn
Copy link
Contributor Author

@miq-bot add_label bug, compute/containers

@nimrodshn
Copy link
Contributor Author

@zeari should this be for gaprindashvili?

@nimrodshn nimrodshn force-pushed the default-labels branch 3 times, most recently from a713b90 to f6dde8a Compare December 28, 2017 17:10
@nimrodshn
Copy link
Contributor Author

cc: @simon3z @serenamarie125

@@ -747,10 +747,17 @@ def get_tags_all(category)
classification.entries.each { |e| @edit[:cb_assign][:tags][e.id.to_s] = e.description } if classification
end

DEFAULT_CHARGEBACK_LABELS = ["com.redhat.component"]
Copy link

Choose a reason for hiding this comment

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

This should be seeded from a file but im really not sure whats the correct way 😓

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah.. Let's wait for @himdel to respond on this..

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is just one constant, a file would probably be an overkill, this is fine 👍 (at least until it becomes some structured data, then we can deal with creating a yaml and importing during seed).

Copy link
Contributor

@himdel himdel Jan 2, 2018

Choose a reason for hiding this comment

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

(If this is something that's not really specific to chargeback but to container images in general, maybe the string should live in the model, idk. But I'm OK with this.)

@mzazrivec mzazrivec added the wip label Jan 2, 2018
@himdel
Copy link
Contributor

himdel commented Jan 2, 2018

This should close #2606, right?

@himdel
Copy link
Contributor

himdel commented Jan 2, 2018

Assuming you find a text for the popover, could you move it to a separate partial please?

Just so that we don't have to repeat the whole bit... something like...

app/views/shared/_popover.html.haml

%a{:"data-toggle" => "popover",
   :"data-html"    => "true",
   :"data-content" => text,
   :"data-placement" => "top"}
  %span.pf-blue-info.pficon.pficon-info

:javascript
  $(document).ready(function() {
    $('[data-toggle=popover]').popovers()
  });

and use

render :partial => "shared/popover", :locals => { :text => _("Foo") }

(also, mising i18n now ;))

(If you don't find any text ... well, it didn't have any help text before, and the BZ is not about missing help, so .. not a blocker if we don't add a popover.)

@nimrodshn
Copy link
Contributor Author

@himdel Yeah I too don't see any reference to "help missing" in the BZ only thing is that @serenamarie125 commented about that in the previous PR (#2606) so I didn't want to ignore her comment even though I too don't think it is necessary. I will wait for her to take a peek and give opinion and if it is not necassary I will drop the popover.

@nimrodshn nimrodshn force-pushed the default-labels branch 3 times, most recently from 7b9c3a2 to 2678047 Compare January 4, 2018 11:32
@nimrodshn
Copy link
Contributor Author

nimrodshn commented Jan 4, 2018

@himdel, I removed the popover - It is not necessary for the BZ. PTAL?
(Updated the GIF ⏫ )

@nimrodshn nimrodshn changed the title [WIP] Adding default labels 'com.redhat.component' to chargback assignments Adding default labels 'com.redhat.component' to chargback assignments Jan 4, 2018
@miq-bot miq-bot removed the wip label Jan 4, 2018
fix-minor mistakes

missing default options

freeze constant

removing popover and adding comments
@miq-bot
Copy link
Member

miq-bot commented Jan 8, 2018

Checked commit nimrodshn@55af067 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

@himdel himdel self-assigned this Jan 8, 2018
@himdel himdel added this to the Sprint 77 Ending Jan 15, 2018 milestone Jan 8, 2018
@himdel himdel merged commit f5e4dfc into ManageIQ:master Jan 8, 2018
@himdel
Copy link
Contributor

himdel commented Jan 8, 2018

LGTM, merged :)

(we probably still want the popover, but didn't want to block the PR on an unrelated change)

@nimrodshn Is this meant for gaprindashvili?

@nimrodshn
Copy link
Contributor Author

BZ is on cfme-future so nope.

@simaishi
Copy link
Contributor

BZ is now marked for z-stream. This can be marked as gaprindashvili/yes?

@nimrodshn
Copy link
Contributor Author

yes 👍 @simaishi

simaishi pushed a commit that referenced this pull request Mar 7, 2018
Adding default labels 'com.redhat.component' to chargback assignments
(cherry picked from commit f5e4dfc)

https://bugzilla.redhat.com/show_bug.cgi?id=1552704
@simaishi
Copy link
Contributor

simaishi commented Mar 7, 2018

Gaprindashvili backport details:

$ git log -1
commit bdfd80c2f246af517f34597addced64e032618af
Author: Martin Hradil <[email protected]>
Date:   Mon Jan 8 17:28:44 2018 +0100

    Merge pull request #3144 from nimrodshn/default-labels
    
    Adding default labels 'com.redhat.component' to chargback assignments
    (cherry picked from commit f5e4dfc220c0e0b3045a1c7b9b5d7c61496ae8f7)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1552704

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants