Github action now reports state of execution #3531 #3531

rasjani · 2023-01-30T08:18:39Z

Description

Adds 2 new outputs to github actions which would allow pipeline maintainer to do conditional steps based on actions black did. These variables include is_formatted which defaults to 0 if no changes where made and 1 when files where formatted. Another one is changed_files, which defaults to 0 but reports the number of files changed during the action execution.

Checklist - did you ...

Add an entry in CHANGES.md if necessary?
Add / update tests if necessary?
Add new / update outdated documentation?

cooperlees

Thanks for the contribution. Looks good. I think there is a few things we can clean up here tho. Lets try these, if any of my suggestions don't work, I apologize, just say so.

action.yml

cooperlees · 2023-01-30T23:39:37Z

action/main.py

@@ -38,8 +44,23 @@
 base_cmd = [str(ENV_BIN / "black")]
 if BLACK_ARGS:
    # TODO: remove after a while since this is deprecated in favour of SRC + OPTIONS.
-    proc = run([*base_cmd, *shlex.split(BLACK_ARGS)])
+    proc = run([*base_cmd, *shlex.split(BLACK_ARGS)], stdout=PIPE, stderr=STDOUT)


Suggested change

proc = run([*base_cmd, *shlex.split(BLACK_ARGS)], stdout=PIPE, stderr=STDOUT)

proc = run([*base_cmd, *shlex.split(BLACK_ARGS)], stderr=PIPE, encoding="utf8")

All the output you're after here goes to stderr. You can also set the encoding on the call.

Regarding what @ichard26 suggested below, if terminal session here is not utf-8 (which is common in windows?), and i'd re-emit the stderr back to console, wouldn't decoding to utf-8 and then re-encoding back to whatever sys.stdout.encoding was cause possible issues ? Not sure about that - asking for clarification

docs/integrations/github_actions.md

cooperlees · 2023-01-30T23:45:54Z

action/main.py

+    proc = run(
+        [*base_cmd, *shlex.split(OPTIONS), *shlex.split(SRC)],
+        stdout=PIPE,
+        stderr=STDOUT,
+    )


Suggested change

proc = run(

[*base_cmd, *shlex.split(OPTIONS), *shlex.split(SRC)],

stdout=PIPE,

stderr=STDOUT,

)

proc = run(

[*base_cmd, *shlex.split(OPTIONS), *shlex.split(SRC)],

stderr=PIPE,

encoding="utf8",

)

cooperlees · 2023-01-30T23:46:38Z

action/main.py

+_output = proc.stdout.decode("utf-8")
+
+matches = search(_is_formatted_re, _output, MULTILINE)


Suggested change

_output = proc.stdout.decode("utf-8")

matches = search(_is_formatted_re, _output, MULTILINE)

matches = search(_is_formatted_re, proc.stderr, MULTILINE)

ichard26

Thanks for submitting a PR but as it stands currently, this hurts UX in the general case. This captures the output of the Black invocation and never reemits it. That's already not great, but it gets worse. If Black errors out for some reason, the developer reading the logs will have no idea what's wrong since the logs will be almost entirely empty. There are two ways of fixing this:

Reemit the captured output (always or on a non-zero return code; I'd prefer the former)
Tee or somehow duplicate the output stream so it can be captured and printed to the console as usual

The second could preserve the colour output and allow for live feedback (unlike the first option), however, I have never done this before so no clue whether this is possible. ASNI codes make it sound really annoying.

I know this Action's output could be improved in general, but that's dependant on GitHub fixing their own infrastructure (which will never happen IME), but if it's possible let's not make it worse.

Also, the general convention for GitHub Actions are to use dashes instead of underscores. Here's an example..

Finally, I'm not keen on using using numbers as boolean, we could use true / false instead. I can go either way on this point though.

I know this is a lot of words, but I do really mean it that I appreciate the effort you've put into this. I'm just one of those "let's right it the first time" folks (un)fortunately :)

CHANGES.md

rasjani · 2023-01-31T13:06:03Z

action/main.py

+# Re-emit stderr back to console so that action output is visible to pipeline
+# Do note, click will strip terminal control codes if the output is not TTY
+# and thus, this will not show colors anymore.
+print(proc.stderr, file=sys.stderr, flush=True)


Im still wondering if i should proc.stderr should match sys.stdout.encoding ?

and to give a bit of background, character encoding on github runners with windows as os, is reported to be cp1252 while ubuntu and macos default to utf-8 .. enforcing utf-8 on subprocess.run() feels dirty if the output is going back to windows runner ..

I guess, using sys.stdout.encoding is prob the better thing to do

Is there a sys.stderr.encoding? - I just know all our output will always be utf8 compliant ...

Since I did miss this swallowing of the output, it's prob better to not encode @ subprocess (like you had it - sorry) and only decode for the regex and reprint the raw bytes. That might preserve the colors etc. too

cooperlees · 2023-01-31T18:01:15Z

Another option would be to have black itself check if it's running in a GitHub action environment and if so dump a file or set environment variable(s) actions could use maybe?

Then we don't mess with stderr that's designed for humans, not computers, at all.

rasjani · 2023-01-31T18:34:55Z

Another option would be to have black itself check if it's running in a GitHub action environment and if so dump a file or set environment variable(s) actions could use maybe?

Not against rewriting the patch for that if the approach is acceptable ? Less issues to tackle for sure but personal take; approach doesn't feel like "keeping it simple".

Anyhuu, I'll make another pr against the core black and if that approach is valid, this pr can be closed.

cooperlees · 2023-01-31T18:43:04Z

Anyhuu, I'll make another pr against the core black and if that approach is valid, this pr can be closed.

Lets give it a few hours for others to see if they like the approach better ... I don't want to eat your time ... I'm also for going this way of people are happy about it ...

I just floated my idea as alarm bells always go off in my head when automation is using output designed for humans / wrapping tools if it can be avoided.

rasjani · 2023-02-01T10:37:10Z

@cooperlees rasjani@367beb8 shows alternative approach baked directly into black itself and not into action ..

And don't worry about wasting my time - at the moment - im "in between" customers and doing foss contributions is considered as positive way to spend time on the bench :)

cooperlees · 2023-02-02T17:57:04Z

Your POC commit looks good to me. I feel lets do that and read from the dumped file (I think JSON is a better choice) leaving stderr alone :)

cooperlees requested changes Jan 30, 2023

View reviewed changes

ichard26 requested changes Jan 31, 2023

View reviewed changes

CHANGES.md Outdated Show resolved Hide resolved

rasjani added a commit to rasjani/black that referenced this pull request Jan 31, 2023

Github action now reports state of execution psf#3531

87ce090

rasjani force-pushed the issue-3526 branch from 849c7d3 to 87ce090 Compare January 31, 2023 13:00

rasjani commented Jan 31, 2023

View reviewed changes

rasjani added a commit to rasjani/black that referenced this pull request Jan 31, 2023

Github action now reports state of execution psf#3531

3716f89

rasjani force-pushed the issue-3526 branch from 87ce090 to 3716f89 Compare January 31, 2023 14:00

Github action now reports state of execution psf#3531

e1c2c0f

rasjani force-pushed the issue-3526 branch from 3716f89 to e1c2c0f Compare February 1, 2023 10:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Github action now reports state of execution #3531 #3531

Github action now reports state of execution #3531 #3531

rasjani commented Jan 30, 2023 •

edited

Loading

cooperlees left a comment

cooperlees Jan 30, 2023

rasjani Jan 31, 2023

cooperlees Jan 30, 2023

cooperlees Jan 30, 2023

ichard26 left a comment

rasjani Jan 31, 2023 •

edited

Loading

rasjani Jan 31, 2023

cooperlees Jan 31, 2023

cooperlees Jan 31, 2023

cooperlees commented Jan 31, 2023 •

edited

Loading

rasjani commented Jan 31, 2023

cooperlees commented Jan 31, 2023

rasjani commented Feb 1, 2023 •

edited

Loading

cooperlees commented Feb 2, 2023

	proc = run([base_cmd, shlex.split(BLACK_ARGS)], stdout=PIPE, stderr=STDOUT)
	proc = run([base_cmd, shlex.split(BLACK_ARGS)], stderr=PIPE, encoding="utf8")

		_output = proc.stdout.decode("utf-8")

		matches = search(_is_formatted_re, _output, MULTILINE)

Github action now reports state of execution #3531 #3531

Are you sure you want to change the base?

Github action now reports state of execution #3531 #3531

Conversation

rasjani commented Jan 30, 2023 • edited Loading

Description

Checklist - did you ...

cooperlees left a comment

Choose a reason for hiding this comment

cooperlees Jan 30, 2023

Choose a reason for hiding this comment

rasjani Jan 31, 2023

Choose a reason for hiding this comment

cooperlees Jan 30, 2023

Choose a reason for hiding this comment

cooperlees Jan 30, 2023

Choose a reason for hiding this comment

ichard26 left a comment

Choose a reason for hiding this comment

rasjani Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

rasjani Jan 31, 2023

Choose a reason for hiding this comment

cooperlees Jan 31, 2023

Choose a reason for hiding this comment

cooperlees Jan 31, 2023

Choose a reason for hiding this comment

cooperlees commented Jan 31, 2023 • edited Loading

rasjani commented Jan 31, 2023

cooperlees commented Jan 31, 2023

rasjani commented Feb 1, 2023 • edited Loading

cooperlees commented Feb 2, 2023

rasjani commented Jan 30, 2023 •

edited

Loading

rasjani Jan 31, 2023 •

edited

Loading

cooperlees commented Jan 31, 2023 •

edited

Loading

rasjani commented Feb 1, 2023 •

edited

Loading