You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Eason09053360 (via GitHub)" <gi...@apache.org> on 2026/09/08 12:33:18 UTC

[PR] Fix airflow info --file-io uploading an empty report [airflow]

Eason09053360 opened a new pull request, #72707:
URL: https://github.com/apache/airflow/pull/72707

   `airflow info --file-io` reports success and hands back a file.io link, but the file it uploaded is
   empty. The command exists only to produce that report, so every link shared from it — typically in a
   bug report — points at a blank file.
   
   ## Cause
   
   `AirflowInfo.render_text()` enabled two different rich mechanisms at once and read back from the one
   that was left empty:
   
   ```python
   console = AirflowConsole(record=True)   # recording: print as usual, keep a copy
   with console.capture():                 # capture: divert output into a buffer
       self.show(output=output, console=console)
   return console.export_text()            # read the recording
   ```
   
   rich only appends to the record buffer while no capture is active
   (`Console._write_buffer`: `if self.record and not self._buffer_index`), and `capture()` increments
   `_buffer_index` on entry. So the render went into the capture buffer, which was never read, and the
   recording stayed empty — `export_text()` returned `""`.
   
   ## Fix
   
   Use the capture alone and read it back. `--file-io` should not echo the whole report to the terminal,
   so capture is the mechanism that matches the intent; recording is not.
   
   The console is also built with `color_system=None`. `export_text()` defaults to `styles=False` and
   stripped styles, while `Capture.get()` renders them, so keeping the default `color_system="auto"`
   would have put ANSI escape sequences into an uploaded file. Measured on a terminal, the `json` output
   carried roughly 5000 bytes of escape codes (20091 → 15168 characters once disabled); `table` and
   `yaml` were affected too.
   
   ## Tests
   
   - `test_show_info_anonymize_fileio` gains an assertion on the body actually handed to `httpx.post`.
     The test previously only checked that the returned link was printed, which is why an empty upload
     went unnoticed. Reverting the fix makes it fail with `assert '3.4.0' in ''` — that empty string is
     what users were uploading.
   - `test_render_text_stays_plain_on_a_color_terminal` sets `FORCE_COLOR=1` so rich treats itself as a
     colour terminal, then asserts the rendered report contains no escape sequences. Without the
     environment variable the assertion would pass regardless, since pytest's stdout is not a tty.
   
   Reverting only `color_system=None` fails the second test; reverting the whole change fails both.
   
   ## Known gap: `--output plain` is only partly fixed
   
   `AirflowConsole.print_as_plain_table` (`airflow-core/src/airflow/cli/simple_table.py:84`) ends in the
   builtin `print(output)` rather than `self.print(...)`, so the tabulated body bypasses the capture and
   goes straight to the terminal. With `--output plain` the upload therefore still contains only the
   section headers (measured: 124 characters captured, 16180 written to stdout).
   
   This is not a regression from this PR — that path uploaded `""` before — but it is not fully fixed
   either. It is left out deliberately, because `print_as_plain_table` is shared by every command's
   `--output plain` and a safe replacement needs four separate flags, each guarding a distinct
   corruption:
   
   | Replacement | Result |
   | --- | --- |
   | `self.print(output)` | `[team-a, prod]` is parsed as rich markup and disappears |
   | `+ markup=False, highlight=False` | byte-identical to `print(output)` |
   | `report:smile:daily` | becomes `report😄daily` without `emoji=False` |
   | narrow terminal | rich wraps where `print` does not, needing `soft_wrap=True` |
   
   That belongs in its own change against `simple_table.py`, with its own coverage for each flag.
   
   ---
   
   ##### Was generative AI tooling used to co-author this PR?
   
   - [X] Yes — Claude Code (Opus 5)
   
   Generated-by: Claude Code (Opus 5) following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Fix airflow info --file-io uploading an empty report [airflow]

Posted by "henry3260 (via GitHub)" <gi...@apache.org>.
henry3260 merged PR #72707:
URL: https://github.com/apache/airflow/pull/72707


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Fix airflow info --file-io uploading an empty report [airflow]

Posted by "rjgoyln (via GitHub)" <gi...@apache.org>.
rjgoyln commented on code in PR #72707:
URL: https://github.com/apache/airflow/pull/72707#discussion_r3970437489


##########
airflow-core/tests/unit/cli/commands/test_info_command.py:
##########
@@ -162,6 +162,15 @@ def test_show_info_anonymize(self, stdout_capture):
         assert airflow_version in output
         assert "postgresql+psycopg2://p...s:PASSWORD@postgres/airflow" in output
 
+    @mock.patch.dict(os.environ, {"FORCE_COLOR": "1"})

Review Comment:
   `FORCE_COLOR=1` alone doesn't guarantee a color system: `_detect_color_system()` returns `None` when `TERM` is `dumb`/`unknown`, so this passes even with `color_system=None` reverted. Measured with `color_system="auto"` - `TERM` unset gives `standard` (escapes present), `TERM=dumb` gives `None` (no escapes). Pinning the terminal type makes it deterministic:
   
   ```suggestion
       @mock.patch.dict(os.environ, {"FORCE_COLOR": "1", "TERM": "xterm-256color"})
   ```



##########
airflow-core/tests/unit/cli/commands/test_info_command.py:
##########
@@ -162,6 +162,15 @@ def test_show_info_anonymize(self, stdout_capture):
         assert airflow_version in output
         assert "postgresql+psycopg2://p...s:PASSWORD@postgres/airflow" in output
 
+    @mock.patch.dict(os.environ, {"FORCE_COLOR": "1"})
+    def test_render_text_stays_plain_on_a_color_terminal(self):

Review Comment:
   First test here to go through the full `show()` path, so it initialises the `ProvidersManager` singleton. The sibling `--file-io` test takes `cleanup_providers_manager` for that reason - without it the initialised provider configuration leaks into whatever runs next on the same worker.
   
   ```suggestion
       def test_render_text_stays_plain_on_a_color_terminal(self, cleanup_providers_manager):
   ```



##########
airflow-core/src/airflow/cli/commands/info_command.py:
##########
@@ -335,10 +335,11 @@ def show(self, output: str, console: AirflowConsole | None = None) -> None:
 
     def render_text(self, output: str) -> str:
         """Export the info to string."""
-        console = AirflowConsole(record=True)
-        with console.capture():
+        # No color_system: the captured text is uploaded as a file, so escape codes are noise.
+        console = AirflowConsole(color_system=None)

Review Comment:
   Optional, same reasoning as `color_system`: `_width` is pinned to 200 only when not a tty, so on a real terminal the uploaded report is laid out at the terminal's width - at 80 columns the long `system_path` / `python_path` values wrap across the table borders. `width=200` would make the uploaded file independent of where it was generated.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Fix airflow info --file-io uploading an empty report [airflow]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #72707:
URL: https://github.com/apache/airflow/pull/72707#issuecomment-5606591752

   Hi maintainer, this PR was merged without a milestone set.
   We've automatically set the milestone to **[Airflow 3.3.2](https://github.com/apache/airflow/milestone/150)** based on: backport label targeting v3-3-test
   If this milestone is not correct, please update it to the appropriate milestone.
   
   > This comment was generated by [Milestone Tag Assistant](https://github.com/apache/airflow/blob/main/.github/workflows/milestone-tag-assistant.yml).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Fix airflow info --file-io uploading an empty report [airflow]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #72707:
URL: https://github.com/apache/airflow/pull/72707#issuecomment-5606598559

   ### Backport successfully created: v3-3-test
   
   Note: As of [Merging PRs targeted for Airflow 3.X](https://github.com/apache/airflow/blob/main/dev/README_AIRFLOW3_DEV.md#merging-prs-targeted-for-airflow-3x)
   the committer who merges the PR is responsible for backporting the PRs that are bug fixes (generally speaking) to the maintenance branches.
   
   In matter of doubt please ask in [#release-management](https://apache-airflow.slack.com/archives/C03G9H97MM2) Slack channel.
   
   <table>
           <tr>
           <th>Status</th>
           <th>Branch</th>
           <th>Result</th>
       </tr>
       <tr>
           <td>✅</td>
           <td>v3-3-test</td>
           <td><a href="https://github.com/apache/airflow/pull/72832"><img src="https://img.shields.io/badge/PR-72832-blue" alt="PR Link"></a></td>
       </tr>
   </table>


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Fix airflow info --file-io uploading an empty report [airflow]

Posted by "Eason09053360 (via GitHub)" <gi...@apache.org>.
Eason09053360 commented on code in PR #72707:
URL: https://github.com/apache/airflow/pull/72707#discussion_r3970859636


##########
airflow-core/tests/unit/cli/commands/test_info_command.py:
##########
@@ -162,6 +162,15 @@ def test_show_info_anonymize(self, stdout_capture):
         assert airflow_version in output
         assert "postgresql+psycopg2://p...s:PASSWORD@postgres/airflow" in output
 
+    @mock.patch.dict(os.environ, {"FORCE_COLOR": "1"})

Review Comment:
   Applied! 
   Thanks



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Fix airflow info --file-io uploading an empty report [airflow]

Posted by "Eason09053360 (via GitHub)" <gi...@apache.org>.
Eason09053360 commented on PR #72707:
URL: https://github.com/apache/airflow/pull/72707#issuecomment-5605564744

   Thanks @rjgoyln for the careful trace, applied two of the three:
   
   - **`TERM` pin** - applied via the suggestion. Confirmed the gap you described: with the fix reverted, `TERM=dumb` made the test pass, and `TERM` unset / `xterm-256color` made it fail. With the pin it fails under all three.
   - **`width=200`** - applied. Reproduced it: on an 80-column tty the uploaded report had `system_path` / `python_path` cut at column 80.
   - **`cleanup_providers_manager`** - left as is. `test_show_info` and `test_show_info_anonymize` in the same class already go through the full `show()` path without that fixture, so the new test is not the first one and I'd rather keep the three consistent. Happy to add it to all three if a maintainer prefers.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org