-
Notifications
You must be signed in to change notification settings - Fork 41
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
Switch from Logify to mixlib-log for logging #150
Conversation
a42c1a0
to
9d3cdcf
Compare
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.
Code looks reasonable to me, the pipeline is complaining and I don't have a way to test this change so, I would like someone else to take a look before merging.
Reduce the number of dependencies in our overall stack and use the same logging mechanism we use in other Chef CLI tools. This formatting matches roughly what we had before. Signed-off-by: Tim Smith <[email protected]>
Signed-off-by: Salim Afiune <[email protected]>
@@ -17,7 +17,7 @@ Cucumber::Rake::Task.new(:acceptance) do |t| | |||
a.push('--color') | |||
a.push('--format progress') | |||
a.push('--strict') | |||
a.push('--tags ~@wip') | |||
a.push('--tags "not @wip"') |
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.
Fixed this so that we don't show this WARN message anymore:
Deprecated: Found tags option '~@wip'. Support for '~@tag' will be removed from the next release of Cucumber. Please use 'not @tag' instead.
@@ -28,6 +28,7 @@ Feature: git Plugin | |||
* I successfully run `stove` | |||
* the git remote should have the tag "v0.0.0" | |||
|
|||
@wip |
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.
I verified master
and this scenario is also failing there, I'll create a card to fix it but for now,
I have marked as @wip
so that we don't run it.
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.
Issue: #154
log.error(e.class.name) | ||
log.error(e.message) | ||
log.error(e.backtrace.join("\n")) | ||
Stove::Log.init($stderr) |
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.
@tas50 @tyler-ball The problem wasn't in the cucumber tests, but the actual code to log
the error to $stderr
. This line fixed everything. 🥇
Thanks @afiune! |
This reduces the overall number of dependencies we need to maintain and ship to customers.