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

Update make commands to set no-TTY flag #376

Merged
merged 4 commits into from
Feb 25, 2022

Conversation

earmenda
Copy link
Contributor

@earmenda earmenda commented Feb 23, 2022

Closes #375

Code Changes

  • Set --no-TTY for targets called within github actions

Steps to Confirm

  • Confirm this PR includes test output

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Documentation Updated

Description Of Changes

Docker Compose version 2.2.2 used to guess whether to set the no-TTY flag when running the docker compose run command. As of 2.2.3 the flag is never set for you.
See the related PR - docker-compose-9035

To understand the flag better, see the following:
https://stackoverflow.com/questions/30137135/confused-about-docker-t-option-to-allocate-a-pseudo-tty

What I think was happening before was that when github actions ran our docker compose run command it would set the no-TTY flag for us, and when running locally it would not.

I can't say i 100% understand how this flag works or why we don't see output without it but setting it should be the equivalent of our previous behavior.

@earmenda earmenda added the bug Something isn't working label Feb 23, 2022
@earmenda earmenda added this to the fidesctl 1.4.0 milestone Feb 23, 2022
@earmenda earmenda self-assigned this Feb 23, 2022
@earmenda
Copy link
Contributor Author

Related issue:
actions/runner-images#5022

@earmenda earmenda mentioned this pull request Feb 23, 2022
4 tasks
@earmenda
Copy link
Contributor Author

Triggered a test failure through this PR and verified we see output now!
https://github.com/ethyca/fides/runs/5298397726

@earmenda
Copy link
Contributor Author

earmenda commented Feb 23, 2022

The only weird thing about this change is that when you run make pytest-unit it now uses the --no-TTY flag locally when it didnt use to.

This doesn't change the functionality but the interaction with the terminal isn't the same. The new output is no longer color coded and formatting might be a little different.

with -T:
without-T

without -T:
with-T

This doesn't concern me too much though because we don't really use these make targets as part of our dev process. We could write some logic so emulate the old behavior but I don't think it is worth it.

Copy link
Contributor

@SteveDMurphy SteveDMurphy left a comment

Choose a reason for hiding this comment

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

Great find @earmenda ! 🙌🏽

@PSalant726
Copy link
Contributor

Nice work finding this - do you have a link to a GH actions change or announcement that confirms they disable TTY mode in runners now?

We should preserve TTY mode in local terminals. Can we include this flag only in the pr_checks.yml file?

@earmenda
Copy link
Contributor Author

earmenda commented Feb 24, 2022

@PSalant726 I made the changes you suggested. We now only set CI_ARGS when the CI environment variable is set. This is based on this - https://docs.github.com/en/actions/learn-github-actions/environment-variables

Copy link
Contributor

@PSalant726 PSalant726 left a comment

Choose a reason for hiding this comment

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

LGTM!

@earmenda earmenda merged commit 3e3c40b into main Feb 25, 2022
@earmenda earmenda deleted the earmenda-fix-missing-github-docker-compose-output branch February 25, 2022 19:21
ThomasLaPiana pushed a commit that referenced this pull request Aug 17, 2022
* validates date field ranges are sane

* add extra type checking

* use datetimes only in the tests
ThomasLaPiana pushed a commit that referenced this pull request Sep 26, 2022
* validates date field ranges are sane

* add extra type checking

* use datetimes only in the tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docker compose run output is missing from github actions
3 participants