Stop destroying test output with automatic logging #569

sarayourfriend · 2022-03-15T22:09:00Z

Fixes

Description

Stops the destructive logging we were doing but maintains helpful-for-debugging logs in CI.

Testing Instructions

Run just api-testlocal with some tests that will fail (assert True is False) and confirm that it does not log away the test output locally.

I'll push a commit with a failing test to confirm that it does still log in CI

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the default branch of the repository (main) or a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
[N/A] I added or updated tests for the changes I made (if applicable).
[N/A] I added or updated documentation (if applicable).
[x[ I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

AetherUnbound

Looks good, just one note on the GHA flow

AetherUnbound · 2022-03-15T22:45:44Z

.github/workflows/ci_cd.yml

+
+      - name: Print API test failure logs
+        if: {{ failure() }}
+        run: just logs


Should this stop as well, since the previous step failed?

I was going to try debugging whether you have to explicitly exit 1 or something or if the if: {{ failure() }} runs just the "failure" jobs and ignores the rest.

AetherUnbound · 2022-03-15T22:46:15Z

api/test/run_test.sh

@@ -18,8 +18,7 @@ succeeded=$?
 if [[ $succeeded -eq 0 ]]; then
 	printf "${green}:-) All tests passed${endcol}\n"
 else
-    printf "Full system logs:\n"
-    docker-compose logs


So apparently this has never worked for just api-test because docker-compose isn't available within the container 🙃

Ahhh, I use api-testlocal because it's easier to interface with pdb than having to docker attach openverse-web-1 or whatever.

sarayourfriend · 2022-03-16T12:13:04Z

Just marking ready for review to test CI but it's still not confirmed to be working.

sarayourfriend · 2022-03-25T12:09:47Z

Apparently printing the API test logs times out the GitHub actions 💀

I think it'd be best to pipe them to a file and then upload them as an artifact.

sarayourfriend · 2022-03-25T12:29:28Z

Nope, I was wrong. The logs are automatically followed by the justfile. Removing the -f default.

AetherUnbound · 2022-03-25T16:09:15Z

justfile

@@ -44,7 +44,7 @@ recreate:
    @just up "--force-recreate --build"

 # Show logs of all, or named, Docker services
-logs services="" args="-f":
+logs services="" args="":


I don't love this change, as it'll mean having to specify -f every time I run this locally 😕

Could we perhaps have this function similar to the IS_PROD flag, maybe with IS_CI, and that automatically removes the -f?

Yeah, that'd be good! We could use the same IS_CI flag to automatically add -T to the other docker-compose commands as well to avoid having to remember that when adding new actions 🎉

That's a great point!

@sarayourfriend if you're okay with it, I can work on adding IS_CI to this branch 🙂

It's up now, thanks though.

zackkrida · 2022-03-28T12:46:29Z

@obulat could you look at this PR this week?

sarayourfriend · 2022-04-04T05:35:29Z

Rebasing this so the actions actually run and we can verify they work 👍

dhruvkb

Can we also export the logs as an artifact? Having the logs locally to scan through would be massively helpful because the GitHub Actions UI is not that good imo.

Also the PR says "[WIP]" in the title, confirming if that is still the case.

sarayourfriend · 2022-04-06T04:45:37Z

@dhruvkb Sure, I actually did at one point upload them as an artifact. I've put it back now. Will remove the WIP once the actions are passing as expected.

dhruvkb · 2022-04-06T05:49:36Z

Feel free to use any code regarding logs from 0283304 (now removed from the other PR).

sarayourfriend · 2022-04-06T06:02:36Z

@dhruvkb TIL you can download the raw output of actions:

Uploading the logs as an artifact still makes sense though because the raw output includes control characters for color output and makes it very annoying to read 😒

sarayourfriend · 2022-04-06T06:03:38Z

This is ready for review now @AetherUnbound @dhruvkb @obulat

AetherUnbound · 2022-04-06T19:56:21Z

justfile

@@ -44,7 +46,7 @@ recreate:
    @just up "--force-recreate --build"

 # Show logs of all, or named, Docker services
-logs services="" args="-f":
+logs services="" args=(if IS_CI != "" { "" } else { "-f" }):


woah, TIL! Very cool

AetherUnbound · 2022-04-06T19:57:28Z

justfile

@@ -1,5 +1,7 @@
 set dotenv-load := false

+IS_CI := env_var_or_default("CI", "")


TIL here as well! This is great

AetherUnbound · 2022-04-06T19:57:29Z

justfile

@@ -1,5 +1,7 @@
 set dotenv-load := false

+IS_CI := env_var_or_default("CI", "")


TIL here as well! This is great

AetherUnbound

I love it! ✨ 🚀
Small nit: for the API, the steps are print -> upload, but for the ingestion server they're upload -> print. It might be good for consistency to have these both be either one or the other.

dhruvkb

I have a few suggestions:

.github/workflows/ci_cd.yml

api/test/run_test.sh

dhruvkb added ✨ goal: improvement Improvement to an existing user-facing feature 🟩 priority: low Low priority and doesn't need to be rushed 🤖 aspect: dx Concerns developers' experience with the codebase labels Mar 15, 2022

AetherUnbound reviewed Mar 15, 2022

View reviewed changes

sarayourfriend marked this pull request as ready for review March 16, 2022 12:12

sarayourfriend requested a review from a team as a code owner March 16, 2022 12:12

sarayourfriend requested review from AetherUnbound and obulat March 16, 2022 12:12

sarayourfriend changed the title ~~Stop destroying test output with automatic logging~~ [WIP] Stop destroying test output with automatic logging Mar 16, 2022

AetherUnbound reviewed Mar 25, 2022

View reviewed changes

sarayourfriend force-pushed the remove/local-logging-test-failure branch from 9fb2bcf to 39cad50 Compare April 4, 2022 05:34

sarayourfriend force-pushed the remove/local-logging-test-failure branch from cc4c242 to de045ca Compare April 5, 2022 06:05

sarayourfriend mentioned this pull request Apr 5, 2022

Reduce flakiness of just init #627

Merged

7 tasks

dhruvkb reviewed Apr 5, 2022

View reviewed changes

sarayourfriend added 9 commits April 6, 2022 06:42

Stop destroying test output with automatic logging

fb8c451

Log for any failure

2dfb943

REMOVE ME: test code for api test failure

77c2c44

Log on ingestion failure

0aabe8d

Try to fix github action syntax

6e80ef0

Fix lint errors

12d73b1

Fix expression syntax

b00857f

Always print logs in CI and fix api/run_test debugging code

5b3b412

Upload logs instead of printing them and timing out GH action

4410703

sarayourfriend added 4 commits April 6, 2022 06:42

Stop auto-following logs

5822b8e

Just print integration tests

4272148

Always log ingestion logs

8166be2

Print and upload logs always

1594292

sarayourfriend force-pushed the remove/local-logging-test-failure branch from de045ca to 1594292 Compare April 6, 2022 04:45

sarayourfriend added 2 commits April 6, 2022 07:03

Default follow logs unless in CI

f05db87

Fix ingestion server path name

c91a10a

sarayourfriend changed the title ~~[WIP] Stop destroying test output with automatic logging~~ Stop destroying test output with automatic logging Apr 6, 2022

AetherUnbound reviewed Apr 6, 2022

View reviewed changes

justfile

@@ -1,5 +1,7 @@

set dotenv-load := false

IS_CI := env_var_or_default("CI", "")

Copy link

Contributor

AetherUnbound Apr 6, 2022

Choose a reason for hiding this comment

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

TIL here as well! This is great

AetherUnbound approved these changes Apr 6, 2022

View reviewed changes

sarayourfriend enabled auto-merge (squash) April 7, 2022 08:27

dhruvkb suggested changes Apr 8, 2022

View reviewed changes

.github/workflows/ci_cd.yml Show resolved Hide resolved

api/test/run_test.sh Show resolved Hide resolved

api/test/run_test.sh Outdated Show resolved Hide resolved

Fix formatting

d31a55c

sarayourfriend requested a review from dhruvkb April 11, 2022 06:52

dhruvkb approved these changes Apr 11, 2022

View reviewed changes

sarayourfriend merged commit d2bc44c into main Apr 11, 2022

sarayourfriend deleted the remove/local-logging-test-failure branch April 11, 2022 09:46

AetherUnbound mentioned this pull request Apr 12, 2022

Add step to run Django check in CI #638

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop destroying test output with automatic logging #569

Stop destroying test output with automatic logging #569

sarayourfriend commented Mar 15, 2022

AetherUnbound left a comment

AetherUnbound Mar 15, 2022

sarayourfriend Mar 15, 2022

AetherUnbound Mar 15, 2022

sarayourfriend Mar 15, 2022

sarayourfriend commented Mar 16, 2022

sarayourfriend commented Mar 25, 2022

sarayourfriend commented Mar 25, 2022

AetherUnbound Mar 25, 2022

sarayourfriend Mar 25, 2022

AetherUnbound Mar 25, 2022

AetherUnbound Apr 4, 2022

sarayourfriend Apr 6, 2022

zackkrida commented Mar 28, 2022

sarayourfriend commented Apr 4, 2022

dhruvkb left a comment •

edited

Loading

sarayourfriend commented Apr 6, 2022

dhruvkb commented Apr 6, 2022

sarayourfriend commented Apr 6, 2022

sarayourfriend commented Apr 6, 2022

AetherUnbound Apr 6, 2022

AetherUnbound Apr 6, 2022

AetherUnbound Apr 6, 2022

AetherUnbound left a comment

dhruvkb left a comment

		@@ -1,5 +1,7 @@
		set dotenv-load := false

		IS_CI := env_var_or_default("CI", "")

Stop destroying test output with automatic logging #569

Stop destroying test output with automatic logging #569

Conversation

sarayourfriend commented Mar 15, 2022

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

AetherUnbound left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarayourfriend commented Mar 16, 2022

sarayourfriend commented Mar 25, 2022

sarayourfriend commented Mar 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zackkrida commented Mar 28, 2022

sarayourfriend commented Apr 4, 2022

dhruvkb left a comment • edited Loading

Choose a reason for hiding this comment

sarayourfriend commented Apr 6, 2022

dhruvkb commented Apr 6, 2022

sarayourfriend commented Apr 6, 2022

sarayourfriend commented Apr 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AetherUnbound left a comment

Choose a reason for hiding this comment

dhruvkb left a comment

Choose a reason for hiding this comment

dhruvkb left a comment •

edited

Loading