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 Persistent Volume link to ems_container #692

Merged
merged 1 commit into from
Mar 22, 2017

Conversation

zakiva
Copy link
Contributor

@zakiva zakiva commented Mar 15, 2017

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1432117

Fixes broken link http://localhost:3000/ems_container/show/6
to http://localhost:3000/ems_container/6

@zakiva
Copy link
Contributor Author

zakiva commented Mar 15, 2017

@miq-bot add_label compute/containers, bug

@zakiva
Copy link
Contributor Author

zakiva commented Mar 15, 2017

@simon3z @zeari Please review

@simon3z
Copy link
Contributor

simon3z commented Mar 15, 2017

@zakiva was this caused by some sort of refactoring? Can you spot what was the change that broke this and then make sure we don't have any other places that needs to be fixed/updated?

@simon3z
Copy link
Contributor

simon3z commented Mar 16, 2017

@zakiva was this caused by some sort of refactoring? Can you spot what was the change that broke this and then make sure we don't have any other places that needs to be fixed/updated?

@zakiva on top of that, can we also have tests to make sure that this won't happen again?

@miq-bot
Copy link
Member

miq-bot commented Mar 16, 2017

Some comments on commit zakiva@57b0e1f

spec/views/layouts/listnav/_persistent_volume.html.haml_spec.rb

  • ⚠️ - 12 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

@miq-bot
Copy link
Member

miq-bot commented Mar 16, 2017

Checked commit zakiva@57b0e1f with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 1 offense detected

spec/views/layouts/listnav/_persistent_volume.html.haml_spec.rb

@zakiva
Copy link
Contributor Author

zakiva commented Mar 16, 2017

@zakiva was this caused by some sort of refactoring? Can you spot what was the change that broke this and then make sure we don't have any other places that needs to be fixed/updated?
@zakiva on top of that, can we also have tests to make sure that this won't happen again?

@simon3z I think it's related to a pretty old refactor when we started using restful path,
added a test for that. I don't see other places where we have this problem.

Also, I believe we should backport it to Euwe.

@miq-bot add_label euwe/yes

@martinpovolny martinpovolny merged commit 6564c61 into ManageIQ:master Mar 22, 2017
@martinpovolny martinpovolny self-assigned this Mar 22, 2017
@martinpovolny martinpovolny added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 22, 2017
@simaishi
Copy link
Contributor

Euwe backport (to manageiq repo) details:

$ git log -1
commit d445d0329c9cbf8f96a35856bbab4625d0dc46e6
Author: Martin Povolny <[email protected]>
Date:   Wed Mar 22 11:58:07 2017 +0100

    Merge pull request #692 from zakiva/fix_volume_link
    
    Fix Persistent Volume link to ems_container
    (cherry picked from commit 6564c612bf353470dd2e74c201ba63933c3d3547)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1436226

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.

5 participants