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

.github/workflows: ci-core: print logs directly #15374

Closed
wants to merge 1 commit into from

Conversation

jmank88
Copy link
Contributor

@jmank88 jmank88 commented Nov 21, 2024

This indirect log printing is no longer necessary since we have limited standard logs again.

@jmank88 jmank88 requested review from a team as code owners November 21, 2024 21:02
@jmank88 jmank88 requested review from kalverra and erikburt November 21, 2024 21:02
@jmank88 jmank88 marked this pull request as draft November 21, 2024 21:14
@jmank88
Copy link
Contributor Author

jmank88 commented Nov 21, 2024

This is not quite as simple as I first thought, since we still use -json for the scheduled runs

@@ -240,17 +240,10 @@ jobs:
id: run-tests
env:
OUTPUT_FILE: ./output.txt
USE_TEE: false
USE_TEE: ${{ matrix.type.printResults }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

USE_TEE is actually not supported anymore. However, it will use tee if $DEBUG is true.

So we can set debug to true when printResults is true, and it's not a scheduled event:

Suggested change
USE_TEE: ${{ matrix.type.printResults }}
DEBUG: ${{ matrix.type.printResults == 'true' && github.event_name != 'schedule' }}

Could also use the filtered test results when it is a scheduled run, as that uses JSON.

      - name: Print Filtered Test Results
        if: ${{ failure() && needs.filter.outputs.should-run-ci-core == 'true' && steps.run-tests.conclusion == 'failure' && github.event_name == 'schedule' }}
        run: |
          if [[ "${{ matrix.type.printResults }}"  == "true" ]]; then
            cat output.txt | gotestloghelper -ci
          fi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO we should try to simplify this overall. Our intention in this case is to run plain go test without a bunch of options, and in principle we shouldn't have to un-configure things to achieve that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, the pipeline itself and these scripts are too complex.

I'm heads down on the test binary hashing solution, which will likely blow up this pipeline so not sure if it's worth investing a lot of time to simplify the existing one.

Copy link
Contributor

@Tofel Tofel Nov 25, 2024

Choose a reason for hiding this comment

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

@jmank88
Copy link
Contributor Author

jmank88 commented Nov 25, 2024

Closing in favor of #15395

@jmank88 jmank88 closed this Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants