-
Notifications
You must be signed in to change notification settings - Fork 160
Add support for GitHub annotations in GitHub actions #1052
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
base: main
Are you sure you want to change the base?
Conversation
This allows to add more formaters based on the same error list.
This allows GitHub to parse it and display annotations on pull requests.
Use REUSE_OUTPUT_FORMAT to allow overriding lint output formats in certain environments (like CI).
Hopefully this will land via fsfe/reuse-tool#1052.
Wow, amazing PR @nijel ! Thank you for this work. I wonder whether it makes sense to keep Do you know whether this GitHub format of errors is also consumed by other programs? |
|
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.
Some questions and feedback. The PR looks great overall.
I will write a few patch commits to fix the easy stuff that doesn't need discussion.
Environment | ||
----------- | ||
|
||
.. envvar:: REUSE_OUTPUT_FORMAT | ||
|
||
Specifies output format, one of ``plain``, ``lines``, ``github``, ``json`` | ||
|
||
It behaves same as corresponding command line options. |
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.
There should be a documented order of precedence here. If both REUSE_OUTPUT_FORMAT
and a CLI flag are used, which takes precedence?
As written in the code, I believe the env var takes precedence. I do not know if this is desirable behaviour.
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.
Additionally, should quiet
be a valid value here?
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.
Additionally, what should happen if the value of REUSE_OUTPUT_FORMAT
is invalid?
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.
Ah, seems like I didn't notice this feedback, sorry.
With the current implementation, the environment variable takes precedence. This is intentional to allow having command line configuration and override it by environment if executed in environment which might want different output (as GitHub).
Unknown values are simply discarded, but it might be a better approach to issue a warning.
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.
having command line configuration and override it by environment if executed in environment which might want different output (as GitHub).
That's an argument I've seen before, in other contexts, and it's always felt upside-down to me.
Typically the precedence goes config file -> envvar -> commmand-line flags. You can store your configuration in a file, but if need be can still override it by setting variables in the environment. Above all else, though, the arguments specified directly on the command line supersede any other sources.
It's a Principle of Least Astonishment thing. Using examples from some common build systems, if I run a command like,
$ cmake -B _build -S . -DCMAKE_INSTALL_PREFIX=/tmp
then for it to install somewhere else because I happened to have CMAKE_INSTALL_PREFIX="/var/tmp"
set in the environment, then that's a very astonishing invisible override.
By the same token, it was a bug in Rust's Cargo a while back that a command like:
$ CARGO_BUILD_TARGET_DIR='env' cargo c --config myConfig.toml
...Would ignore a target dir specified in myConfig.toml
. So, that was fixed.
BUT, that's actually another example of the previous form (command-line flags overriding the environment), because the config is loaded explicitly via the command-line args. Cargo has always, correctly, allowed a command like this:
$ CARGO_BUILD_TARGET_DIR='env' cargo c
...to override the target dir set in its default, autoloaded .cargo/config.toml
file.
Seems to me that. if you want to set values via the environment, don't put them on the command line. Or, use the envvars on the command line, possibly with defaults.
For example:
make ${BUILD_TARGET:-all}
...will run make all
unless ${BUILD_TARGET}
is set in the environment.
But a make all
should never ignore that explicit all
and build something different, just because there's some value set in a config file or an environment variable. IMHO.
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.
You're right, the precedence should be different. I thought about being able to override it when executed in GitHub actions, but reuse is there executed without a command line argument, so the environment would be used even if it is not a priority.
I see @carmenbianca has now made some changes on top of mine, so I'm not going to touch the PR unless it is desired to not interfere with other changes.
Signed-off-by: Carmen Bianca BAKKER <[email protected]>
I added a rather silly fallback. Maybe an error should be raised, but I don't want to deal with catching this very obscure error. Signed-off-by: Carmen Bianca BAKKER <[email protected]>
Signed-off-by: Carmen Bianca BAKKER <[email protected]>
Signed-off-by: Carmen Bianca BAKKER <[email protected]>
Signed-off-by: Carmen Bianca BAKKER <[email protected]>
The first implementation of this consumed the `reuse lint --json` output. It was later patched up to consume the ProjectReport instead, but the docstrings were apparently never adjusted accordingly. Signed-off-by: Carmen Bianca BAKKER <[email protected]>
--github
to format output according to https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actionsREUSE_OUTPUT_FORMAT
to override output from the environment, this is useful to change output in CI compared to when running in user environment.changelog.d/<directory>/
.AUTHORS.rst
.docs/man/
or elsewhere.the current specification.
changed files.