-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add GitHub Actions annotations #34
Add GitHub Actions annotations #34
Conversation
This is currently very light on tests. I also wonder about the additional class. See voxpupuli/puppet-example#19 for an example: |
def override_env | ||
old_env = ENV.clone | ||
yield | ||
ensure | ||
ENV.clear | ||
ENV.update(old_env) | ||
end |
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 wasn't quite sure about this, but I wrote it on the train without internet access.
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.
Is this comment resolvable?
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.
Well, it works. I just don't know if this is the desired solution in this repo.
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.
looks quite good to me and the screenshot shows it's working.
e1e7e81
to
d91347e
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.
LGTM
It looks like no CI ran, only the CLA check. I'm not sure why that is. |
That’s weird… |
Found it and it's the same thing we suffer from in theforeman: https://github.com/puppetlabs/puppet-lint/actions/workflows/ci.yml shows
This is because there's a schedule and regular PR testing in the same workflow, which is very annoying. |
See, it was good that we enabled CI. Looks like I need to please Rubocop. |
This should be no difference in execution, but it pleases Rubocop in the next commit when some code is added and makes review easier.
d91347e
to
670cbac
Compare
It looks like there's an interesting interaction I didn't foresee: the CI runs on GitHub actions as well so that's generating the warnings in this test suite, which was not intended. Looks like I need to use the configuration option I implemented. |
It is possible to get inline annotations with GitHub Actions. This is based on Rubocop's formatter. It's implemented as a configuration option that's dynamic by default. This means it will show up in actions, but not in other environments. Overriding is still possible.
670cbac
to
b70093d
Compare
override_env do | ||
ENV.delete('GITHUB_ACTION') | ||
subject.defaults | ||
end |
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.
Annotating my own code: technically this is redundant now that there's ENV.delete()
in spec_helper.rb
, but it doesn't hurt to be explicit.
# Disable GitHub Actions reporting since it breaks the test suite | ||
ENV.delete('GITHUB_ACTION') |
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.
This isn't the cleanest, but I couldn't find a good way to consistently set the setting. Right now I don't think it would break anything.
@ekohl Apologies, I forgot to get back to you. This does indeed break PDK rendering rather spectacularly when running with a |
Fascinating. I wonder what it's doing. Is it by any chance using JSON output? |
I believe #35 should fix it, or at least unbreak JSON mode. Could you verify that? |
👍 looks good |
It is possible to get inline annotations with GitHub Actions. This is based on Rubocop's formatter.
It's implemented as a configuration option that's dynamic by default. This means it will show up in actions, but not in other environments. Overriding is still possible.