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 rake evm:status and evm:status_full #17054

Merged

Conversation

NickLaMuro
Copy link
Member

After a botched rebase (allegedly), the evm status commands became unusable since the scope for the mb_usage and mb_threshold was in the incorrect block.

This fixes that, as well as adds some tests to make this quicker to catch instead of letting our users find out for themselves.

Links

Steps for Testing/QA

Confirm we aren't crazy by trying to run rake evm:status on master, then checkout these changes and do the same.

@NickLaMuro
Copy link
Member Author

cc/ @kbrock @Fryguy @carbonin

@NickLaMuro
Copy link
Member Author

@miq-bot add_label bug

@miq-bot miq-bot added the bug label Feb 26, 2018
Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

thnx nick

@NickLaMuro
Copy link
Member Author

NickLaMuro commented Feb 26, 2018

SCREW YOU @miq-bot ! I KNOW WHAT LOOKS GOOD!!1! ❗️ ❗️

On a more serious note, unsure why the factories seem to work differently on my machine and are causing discrepancies in the output, but will look into it. Correction/update: I was reading the test output incorrectly, it was a spacing issue, not missing data.

@NickLaMuro NickLaMuro force-pushed the fix_rake_evm_status_and_add_tests branch from 83f15bc to 26ad721 Compare February 26, 2018 23:15
@NickLaMuro
Copy link
Member Author

Okay, made the testing for this less brittle, so travis should now pass 😓.


These tests #Suck™, and it sucks that I am testing what gets sent to STDOUT/puts, and REALLY don't want to use tableize to test the output of tableize (though, I would suspect that would look ugly too...).

Anyway, my plan is to make evm_application.rb more easily testible in a future PR, so I would say for this PR, either we keep these tests for good measure, or we just take the bug fix and I make the testing happen when I refactor evm_application.rb in the future™.

@kbrock
Copy link
Member

kbrock commented Feb 27, 2018

@NickLaMuro the width of the columns are variable based upon the input.

If you want to avoid tabelize (which does have tests by the way) I'd use "%*s" e.g.:

strs.map { |str| "%-*s" % [strs.map(&:length).max, str] }

@NickLaMuro
Copy link
Member Author

NickLaMuro commented Feb 27, 2018

@kbrock right, I realized the variable width of the columns after I started looking into it "for real-sies".

If you want to avoid tabelize (which does have tests by the way)

yeah, I figured. Mostly wanted someone to see the heredocs and be like "oh, okay, that is what it is supposed to look like". Ideally, the methods that provide the data to tableize, and the methods that call them would be separate, so you could just test the data gathers for accuracy and compare arrays.

Didn't want to do that refactoring as part of this PR, but now thinking that adding the tests is just too brittle...

@NickLaMuro NickLaMuro force-pushed the fix_rake_evm_status_and_add_tests branch from 26ad721 to 5b40504 Compare February 27, 2018 03:31
After a botched rebase (allegedly), the evm status commands became
unusable since the scope for the `mb_usage` and `mb_threshold` was in
the incorrect block.

This fixes that, as well as adds some tests to make this quicker to
catch instead of letting our users find out for themselves.
@miq-bot
Copy link
Member

miq-bot commented Feb 27, 2018

Some comments on commit NickLaMuro@5b40504

spec/lib/tasks/evm_application_spec.rb

  • ⚠️ - 102 - Detected puts. Remove all debugging statements.
  • ⚠️ - 103 - Detected puts. Remove all debugging statements.
  • ⚠️ - 51 - Detected puts. Remove all debugging statements.
  • ⚠️ - 52 - Detected puts. Remove all debugging statements.
  • ⚠️ - 69 - Detected puts. Remove all debugging statements.
  • ⚠️ - 70 - Detected puts. Remove all debugging statements.

@miq-bot
Copy link
Member

miq-bot commented Feb 27, 2018

Checked commit NickLaMuro@5b40504 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 43 offenses detected

spec/lib/tasks/evm_application_spec.rb

@kbrock kbrock merged commit b0701cf into ManageIQ:master Feb 27, 2018
@agrare agrare added this to the Sprint 81 Ending Mar 12, 2018 milestone Feb 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants