Skip to content
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

fix: Fix missing encoding when logging from Makefile #535

Merged
merged 3 commits into from
Aug 30, 2023

Conversation

lucashuy
Copy link
Contributor

Issue #, if available:
N/A

Description of changes:
Some of the output that may come from a Makefile (eg. Terraform output) can contain some colour and encoding information (eg. b'\x1b[0m\x1b[1mInitializing provider plugins...\x1b[0m\n' ). This does not get logged properly in the terminal.

Changes are to use sys.stderr.buffer.write() to print these messages instead.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@lucashuy lucashuy marked this pull request as ready for review August 29, 2023 20:17
@lucashuy lucashuy requested a review from a team as a code owner August 29, 2023 20:17
@@ -92,9 +93,11 @@ def run(self, args, env=None, cwd=None):

# Log every stdout line by iterating
for line in p.stdout:
decoded_line = line.decode("utf-8").strip()
LOG.info(decoded_line)
sys.stderr.buffer.write(line)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, sys.stderr.buffer.write won't include a newline at the end of the string which might make the next log statement messy. Should we add a newline after?

Copy link
Contributor Author

@lucashuy lucashuy Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ref:
https://docs.python.org/3/library/io.html#io.IOBase
https://docs.python.org/3/library/io.html#io.IOBase.readline

For the line that gets returned by the iterator (for line in stdout), it calls readline() under the hood and there should always be a new line character at the end of each line. So here, we should be fine to not include a new line of our own.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for digging in!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just can you add a comment why we used sys.stderr to log the terraform output instead of log.info.

@lucashuy
Copy link
Contributor Author

lucashuy commented Aug 29, 2023 via email

@lucashuy lucashuy added this pull request to the merge queue Aug 29, 2023
Merged via the queue into aws:develop with commit b123bed Aug 30, 2023
123 checks passed
hawflau added a commit to hawflau/aws-lambda-builders that referenced this pull request Nov 10, 2023
* Add nodejs20.x support

* fix: Fix missing encoding when logging from Makefile (aws#535)

* Fix missing encoding when logging from Makefile

* Add comment explaining why stderr

* Undo testing code

* Version bump to 1.37.0 (aws#537)

* Support PEP 600 platform tags for arm64 (aws#536)

Co-authored-by: Daniel Mil <[email protected]>

* chore(deps-dev): bump ruff from 0.0.284 to 0.0.287 in /requirements (aws#539)

Bumps [ruff](https://github.com/astral-sh/ruff) from 0.0.284 to 0.0.287.
- [Release notes](https://github.com/astral-sh/ruff/releases)
- [Changelog](https://github.com/astral-sh/ruff/blob/main/BREAKING_CHANGES.md)
- [Commits](astral-sh/ruff@v0.0.284...v0.0.287)

---
updated-dependencies:
- dependency-name: ruff
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore(deps): bump actions/checkout from 3 to 4 (aws#538)

Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v3...v4)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore(deps-dev): bump black from 23.3.0 to 23.7.0 in /requirements (aws#534)

* chore(deps-dev): bump black from 23.3.0 to 23.7.0 in /requirements

Bumps [black](https://github.com/psf/black) from 23.3.0 to 23.7.0.
- [Release notes](https://github.com/psf/black/releases)
- [Changelog](https://github.com/psf/black/blob/main/CHANGES.md)
- [Commits](psf/black@23.3.0...23.7.0)

---
updated-dependencies:
- dependency-name: black
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

* Update requirements/dev.txt

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Wing Fung Lau <[email protected]>

* chore(deps-dev): bump black from 23.7.0 to 23.9.1 in /requirements (aws#543)

Bumps [black](https://github.com/psf/black) from 23.7.0 to 23.9.1.
- [Release notes](https://github.com/psf/black/releases)
- [Changelog](https://github.com/psf/black/blob/main/CHANGES.md)
- [Commits](psf/black@23.7.0...23.9.1)

---
updated-dependencies:
- dependency-name: black
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore(deps-dev): bump ruff from 0.0.287 to 0.0.288 in /requirements (aws#542)

Bumps [ruff](https://github.com/astral-sh/ruff) from 0.0.287 to 0.0.288.
- [Release notes](https://github.com/astral-sh/ruff/releases)
- [Changelog](https://github.com/astral-sh/ruff/blob/main/BREAKING_CHANGES.md)
- [Commits](astral-sh/ruff@v0.0.287...v0.0.288)

---
updated-dependencies:
- dependency-name: ruff
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore(deps-dev): update pyelftools requirement in /requirements (aws#540)

Updates the requirements on [pyelftools](https://github.com/eliben/pyelftools) to permit the latest version.
- [Changelog](https://github.com/eliben/pyelftools/blob/master/CHANGES)
- [Commits](eliben/pyelftools@v0.29...v0.30)

---
updated-dependencies:
- dependency-name: pyelftools
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore(deps-dev): bump coverage from 7.2.7 to 7.3.1 in /requirements (aws#541)

* chore(deps-dev): bump coverage from 7.2.7 to 7.3.1 in /requirements

Bumps [coverage](https://github.com/nedbat/coveragepy) from 7.2.7 to 7.3.1.
- [Release notes](https://github.com/nedbat/coveragepy/releases)
- [Changelog](https://github.com/nedbat/coveragepy/blob/master/CHANGES.rst)
- [Commits](nedbat/coveragepy@7.2.7...7.3.1)

---
updated-dependencies:
- dependency-name: coverage
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

* fix: set coverage versions based on python versions

- since coverage 7.3.x python3.7 support has been dropped.

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Sriram Madapusi Vasudevan <[email protected]>
Co-authored-by: Sriram Madapusi Vasudevan <[email protected]>

* chore: Version Bump 1.38.0 (aws#544)

* chore(deps-dev): bump ruff from 0.0.288 to 0.0.290 in /requirements (aws#545)

Bumps [ruff](https://github.com/astral-sh/ruff) from 0.0.288 to 0.0.290.
- [Release notes](https://github.com/astral-sh/ruff/releases)
- [Changelog](https://github.com/astral-sh/ruff/blob/main/BREAKING_CHANGES.md)
- [Commits](astral-sh/ruff@v0.0.288...v0.0.290)

---
updated-dependencies:
- dependency-name: ruff
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Update github action to install nodejs 20

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Lucas <[email protected]>
Co-authored-by: Daniel Mil <[email protected]>
Co-authored-by: jack-davies <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Wing Fung Lau <[email protected]>
Co-authored-by: Sriram Madapusi Vasudevan <[email protected]>
Co-authored-by: Sriram Madapusi Vasudevan <[email protected]>
Co-authored-by: Mohamed Elasmar <[email protected]>
Co-authored-by: Mohamed ElAsmar <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Nov 10, 2023
* Add nodejs20.x support

* fix: Fix missing encoding when logging from Makefile (#535)

* Fix missing encoding when logging from Makefile

* Add comment explaining why stderr

* Undo testing code

* Version bump to 1.37.0 (#537)

* Support PEP 600 platform tags for arm64 (#536)



* chore(deps-dev): bump ruff from 0.0.284 to 0.0.287 in /requirements (#539)

Bumps [ruff](https://github.com/astral-sh/ruff) from 0.0.284 to 0.0.287.
- [Release notes](https://github.com/astral-sh/ruff/releases)
- [Changelog](https://github.com/astral-sh/ruff/blob/main/BREAKING_CHANGES.md)
- [Commits](astral-sh/ruff@v0.0.284...v0.0.287)

---
updated-dependencies:
- dependency-name: ruff
  dependency-type: direct:development
  update-type: version-update:semver-patch
...




* chore(deps): bump actions/checkout from 3 to 4 (#538)

Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v3...v4)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-major
...




* chore(deps-dev): bump black from 23.3.0 to 23.7.0 in /requirements (#534)

* chore(deps-dev): bump black from 23.3.0 to 23.7.0 in /requirements

Bumps [black](https://github.com/psf/black) from 23.3.0 to 23.7.0.
- [Release notes](https://github.com/psf/black/releases)
- [Changelog](https://github.com/psf/black/blob/main/CHANGES.md)
- [Commits](psf/black@23.3.0...23.7.0)

---
updated-dependencies:
- dependency-name: black
  dependency-type: direct:development
  update-type: version-update:semver-minor
...



* Update requirements/dev.txt

---------





* chore(deps-dev): bump black from 23.7.0 to 23.9.1 in /requirements (#543)

Bumps [black](https://github.com/psf/black) from 23.7.0 to 23.9.1.
- [Release notes](https://github.com/psf/black/releases)
- [Changelog](https://github.com/psf/black/blob/main/CHANGES.md)
- [Commits](psf/black@23.7.0...23.9.1)

---
updated-dependencies:
- dependency-name: black
  dependency-type: direct:development
  update-type: version-update:semver-minor
...




* chore(deps-dev): bump ruff from 0.0.287 to 0.0.288 in /requirements (#542)

Bumps [ruff](https://github.com/astral-sh/ruff) from 0.0.287 to 0.0.288.
- [Release notes](https://github.com/astral-sh/ruff/releases)
- [Changelog](https://github.com/astral-sh/ruff/blob/main/BREAKING_CHANGES.md)
- [Commits](astral-sh/ruff@v0.0.287...v0.0.288)

---
updated-dependencies:
- dependency-name: ruff
  dependency-type: direct:development
  update-type: version-update:semver-patch
...




* chore(deps-dev): update pyelftools requirement in /requirements (#540)

Updates the requirements on [pyelftools](https://github.com/eliben/pyelftools) to permit the latest version.
- [Changelog](https://github.com/eliben/pyelftools/blob/master/CHANGES)
- [Commits](eliben/pyelftools@v0.29...v0.30)

---
updated-dependencies:
- dependency-name: pyelftools
  dependency-type: direct:development
...




* chore(deps-dev): bump coverage from 7.2.7 to 7.3.1 in /requirements (#541)

* chore(deps-dev): bump coverage from 7.2.7 to 7.3.1 in /requirements

Bumps [coverage](https://github.com/nedbat/coveragepy) from 7.2.7 to 7.3.1.
- [Release notes](https://github.com/nedbat/coveragepy/releases)
- [Changelog](https://github.com/nedbat/coveragepy/blob/master/CHANGES.rst)
- [Commits](nedbat/coveragepy@7.2.7...7.3.1)

---
updated-dependencies:
- dependency-name: coverage
  dependency-type: direct:development
  update-type: version-update:semver-minor
...



* fix: set coverage versions based on python versions

- since coverage 7.3.x python3.7 support has been dropped.

---------






* chore: Version Bump 1.38.0 (#544)

* chore(deps-dev): bump ruff from 0.0.288 to 0.0.290 in /requirements (#545)

Bumps [ruff](https://github.com/astral-sh/ruff) from 0.0.288 to 0.0.290.
- [Release notes](https://github.com/astral-sh/ruff/releases)
- [Changelog](https://github.com/astral-sh/ruff/blob/main/BREAKING_CHANGES.md)
- [Commits](astral-sh/ruff@v0.0.288...v0.0.290)

---
updated-dependencies:
- dependency-name: ruff
  dependency-type: direct:development
  update-type: version-update:semver-patch
...




* Update github action to install nodejs 20

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Andrea Culot <[email protected]>
Co-authored-by: Lucas <[email protected]>
Co-authored-by: Daniel Mil <[email protected]>
Co-authored-by: jack-davies <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Sriram Madapusi Vasudevan <[email protected]>
Co-authored-by: Sriram Madapusi Vasudevan <[email protected]>
Co-authored-by: Mohamed Elasmar <[email protected]>
Co-authored-by: Mohamed ElAsmar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants