-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
#10506 - Machine Readable Global-Status #10884
#10506 - Machine Readable Global-Status #10884
Conversation
Hey there @GregJPreece - Thanks for taking the time to make a pull request with Vagrant! While generally I do agree that the One enhancement that could be worth making is having a difference between "messages" and general output, but I'm not entirely sure how to do that without adding more columns and keeping it consistent with how it works now 🤔 |
Indeed, as mentioned in my comment I'm not myself in favour of breaking current default behaviour, but I'm not familiar enough with Ruby or Vagrant to know how to get a flag check into the UI lib function. Do you have any suggestions? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @GregJPreece, thanks for this PR! I am 👍 on the updates to make the output of the global-status
command more useful when in machine readable mode. I don't think there's any reason to remove ui
outputs (and mentioned that in the review). Otherwise, there's just a few things to be adjusted and this PR should be good to go.
Cheers!
test/unit/vagrant/ui_test.rb
Outdated
true | ||
} | ||
it "does not output UI type to the machine-readable output" do | ||
expect(subject).to receive(:safe_puts).never |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would expect the UI output to still be included within the machine readable output. A new test should also be added which validates the new machine readable content is output as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates. Everything looks great. I did toss in some test coverage on the new behavior, just FYI.
Cheers!
You beat me to it! I was going to figure those out this evening. Thanks for taking care of that for me. I may have similar patches to come, based on my experiences with machine-readable mode & automation. I'll keep the comments raised on this PR in mind for those. |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
As noted in #10506, the
vagrant global-status --machine-readable
command produces a rather unhelpful format for machine processing. It does not specify any@ui.machine
outputs, only@ui.info
, meaning that the formatted table outputted by this command is fudged into a series of machine-readable lines that are difficult to parse and not particularly useful. This PR adds@ui.machine
calls alongside them, to make parsing the output of this command much easier, and consistent with similar commands such asvagrant status --machine-readable
.Before:
After:
Please see the comments for a request for info re: suppressing UI output
I am currently writing a build system wrapper for Vagrant, so I will be using the machine-readable option on most of the available Vagrant commands. If Hashicorp are receptive to patches along similar lines to this one, I would like to help clean up the machine-readable mode and make it more consistent. It's a very useful mode to have, and makes integrating Vagrant with automation/build tools much easier.