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

Fix missing operations for MW datasource. #16611

Merged
merged 1 commit into from
Dec 12, 2017

Conversation

tumido
Copy link
Member

@tumido tumido commented Dec 6, 2017

Add a delegation for MiddewareDatasource mutability to the MiddewareServer similarly as it is for MiddlewareDeployment. This allows to calculate visibility in MiddlewareStandaloneServerAction helper properly and show Operation buttons on MW datasource page.

Fixes: BUG 1460296
Test coverage in related: ManageIQ/manageiq-ui-classic#2972
Regression introduced by: ManageIQ/manageiq-ui-classic@491caa1

Before:
image

After:
image

@tumido
Copy link
Member Author

tumido commented Dec 6, 2017

@miq-bot add_label bug,providers/middleware
cc @ManageIQ/committers-hawkular

@miq-bot
Copy link
Member

miq-bot commented Dec 6, 2017

Checked commit tumido@87f9518 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

@abonas
Copy link
Member

abonas commented Dec 7, 2017

@miq-bot add_label gaprindashvili/no

@abonas
Copy link
Member

abonas commented Dec 7, 2017

@tumido the bug states it's a regression - do you know perhaps what caused it? was this code accidentally deleted in the past? (this can also influence whether we can/should include it in gaprindashvili, although the bug's target release at the moment is set to future)

@cfcosta
Copy link

cfcosta commented Dec 7, 2017

@abonas I think this has never behaved correctly. Mutability implementation was done only for MiddlewareServer, so datasources do not define it.

@tumido
Copy link
Member Author

tumido commented Dec 7, 2017

@abonas, I second @cfcosta, I think it never worked correctly. The line was never there, so any other way how it could work is that the @record context was different (MW server) but I don't see when if ever that happened.

@israel-hdez
Copy link
Member

@tumido I think it's good to have some tests in this repo. Tests does not run across repositories. By having tests only in UI, if this gets broken in the future, we may realize it after it's merged.

@israel-hdez
Copy link
Member

israel-hdez commented Dec 7, 2017

I think it's a regression.
In fine-beta1, the UI helper deciding the visibility of the button was falling back to check immutability on the parent mw server. In gaprindashvili (and in current master), it's only checking with whatever is in @record.

@tumido
Copy link
Member Author

tumido commented Dec 7, 2017

@israel-hdez,

In fine-beta1, the UI helper deciding the visibility of the button was falling back to check immutability on the parent mw server

Nice! That's it! So it's a regression and in my eyes it should land in gaprindashvili

Tests does not run across repositories

Though the functionality is not included in this repo. I can write some tests, but they would only verify the presence of this line, which seems to be useless to me...

@skateman
Copy link
Member

skateman commented Dec 8, 2017

It seems to me as a trivial one-liner and the functionality is in the UI, so tests are also there. Or will be this single line used elsewhere in this repo?

@tumido
Copy link
Member Author

tumido commented Dec 11, 2017

@miq-bot add_label gapridashvili/yes
@miq-bot remove_label gaprindashvili/no

@miq-bot
Copy link
Member

miq-bot commented Dec 11, 2017

@tumido Cannot apply the following label because they are not recognized: gapridashvili/yes

@tumido
Copy link
Member Author

tumido commented Dec 11, 2017

@miq-bot add_label gaprindashvili/yes

Copy link
Contributor

@karelhala karelhala left a comment

Choose a reason for hiding this comment

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

LGTM

@tumido
Copy link
Member Author

tumido commented Dec 11, 2017

Can we get this reviewed, @ManageIQ/committers-hawkular ?

Copy link
Member

@israel-hdez israel-hdez left a comment

Choose a reason for hiding this comment

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

Code looks good.
If you are confident that the UI tests are enough, it's good to go for me.

@abonas
Copy link
Member

abonas commented Dec 12, 2017

@agrare looks like it should go to core now

@agrare agrare self-assigned this Dec 12, 2017
@agrare
Copy link
Member

agrare commented Dec 12, 2017

@tumido @israel-hdez can you find the PR that changed this behavior in the UI just for future reference and post it here?

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@agrare agrare merged commit ef638d5 into ManageIQ:master Dec 12, 2017
@tumido
Copy link
Member Author

tumido commented Dec 12, 2017

@agrare, sure. Regression was introduced by ManageIQ/manageiq-ui-classic@491caa1 . (Description updated as well.)

simaishi pushed a commit that referenced this pull request Dec 12, 2017
Fix missing operations for MW datasource.
(cherry picked from commit ef638d5)
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit ba585f05620382f472655ce7fd11a2fc1704cbec
Author: Adam Grare <agrare@redhat.com>
Date:   Tue Dec 12 08:11:35 2017 -0500

    Merge pull request #16611 from tumido/fix_mw_datasource_operations
    
    Fix missing operations for MW datasource.
    (cherry picked from commit ef638d59a45dc4e2d95795836349edcee7059afa)

@himdel
Copy link
Contributor

himdel commented Dec 12, 2017

FYI, this caused a ui-classic travis failure..

  1) MiddlewareDatasourceController#show render 
     Failure/Error: !@record.try(:in_domain?) && super
     
     ActionView::Template::Error:
       MiddlewareDatasource#in_domain? delegated to middleware_server.in_domain?, but middleware_server is nil: #<ManageIQ::Providers::Hawkular::MiddlewareManager::MiddlewareDatasource id: 63000000000038, name: "ExampleDS", ems_ref: nil, nativeid: "Local~/subsystem=datasources/data-source=ExampleDS", server_id: nil, properties: {"Driver Name"=>"foo", "Connection URL"=>"bar", "JNDI Name"=>"foo-bar", "Enabled"=>"yes"}, ems_id: nil, created_at: "2017-12-12 16:59:57", updated_at: "2017-12-12 16:59:57", feed: nil, type: "ManageIQ::Providers::Hawkular::MiddlewareManager::...">
     # /home/himdel/manageiq/app/models/middleware_datasource.rb:10:in `rescue in in_domain?'
     # /home/himdel/manageiq/app/models/middleware_datasource.rb:10:in `in_domain?'
     # ./app/helpers/application_helper/button/middleware_standalone_server_action.rb:3:in `visible?'
     # ./app/helpers/application_helper/button/basic.rb:77:in `skipped?'
     # ./app/helpers/application_helper/toolbar_builder.rb:81:in `toolbar_button

@agrare
Copy link
Member

agrare commented Dec 12, 2017

@himdel there is an associated ui-classic PR ManageIQ/manageiq-ui-classic#2972 that I assume addresses the issue, can you take a look at that?

@himdel
Copy link
Contributor

himdel commented Dec 12, 2017

@agrare It doesn't :( It changes an entirely different spec file.

But if you know more about the area, can you please look at ManageIQ/manageiq-ui-classic#3040 ? :) This should fix it.

@agrare agrare added this to the Sprint 76 Ending Jan 1, 2018 milestone Dec 12, 2017
simaishi added a commit that referenced this pull request Dec 12, 2017
@simaishi
Copy link
Contributor

Reverted the backport to Gaprindashvili branch.

$ git log -1
commit 448e10ee3c5d78cfda0f2b4138393e9610a2076d
Author: Satoe Imaishi <simaishi@redhat.com>
Date:   Tue Dec 12 14:32:54 2017 -0500

    Revert "Merge pull request #16611 from tumido/fix_mw_datasource_operations"
    
    This reverts commit ba585f05620382f472655ce7fd11a2fc1704cbec.

@tumido
Copy link
Member Author

tumido commented Dec 13, 2017

Please backport together with #16653, which is fixing the broken factory discovered by Travis for manageiq-ui-classic.

simaishi pushed a commit that referenced this pull request Dec 15, 2017
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 555f59bf6d9b80b0b0908987562e1d40511764c9
Author: Adam Grare <agrare@redhat.com>
Date:   Tue Dec 12 08:11:35 2017 -0500

    Merge pull request #16611 from tumido/fix_mw_datasource_operations
    
    Fix missing operations for MW datasource.
    (cherry picked from commit ef638d59a45dc4e2d95795836349edcee7059afa)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1526557

@tumido tumido deleted the fix_mw_datasource_operations branch June 26, 2018 14:17
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.

10 participants