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

[BREAKING] refactor logger #2552

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ChristopherHX
Copy link
Contributor

Set up job and Complete job are now steps in json logs

BREAKING this can break my own embedded use cases

@SanjulaGanepola I have no plan for this PR to land before 6 pending PR's including breaking changes, since this has huge impact for github-act-runner or even act_runner of gitea.

Copy link
Contributor

github-actions bot commented Dec 1, 2024

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ EDITORCONFIG editorconfig-checker 2 0 0.01s
✅ REPOSITORY gitleaks yes no 2.39s
✅ REPOSITORY git_diff yes no 0.01s
✅ REPOSITORY grype yes no 8.52s
✅ REPOSITORY secretlint yes no 1.22s
✅ REPOSITORY trivy-sbom yes no 0.13s
✅ REPOSITORY trufflehog yes no 3.88s

See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

Copy link

codecov bot commented Dec 1, 2024

Codecov Report

Attention: Patch coverage is 63.63636% with 20 lines in your changes missing coverage. Please review.

Project coverage is 68.74%. Comparing base (5a80a04) to head (0cc29b7).
Report is 140 commits behind head on master.

Files with missing lines Patch % Lines
pkg/common/executor.go 53.33% 11 Missing and 3 partials ⚠️
pkg/runner/job_executor.go 76.00% 5 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2552      +/-   ##
==========================================
+ Coverage   61.56%   68.74%   +7.18%     
==========================================
  Files          53       71      +18     
  Lines        9002    10959    +1957     
==========================================
+ Hits         5542     7534    +1992     
+ Misses       3020     2859     -161     
- Partials      440      566     +126     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@SanjulaGanepola SanjulaGanepola left a comment

Choose a reason for hiding this comment

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

I tested the changes with the extension and they work. I see we now have stepResult for setup and complete job steps. Do you think this PR can contain the other 2 items mentioned in the linked issue:

  • stepResult for pre stage
  • Message for skipped jobs/steps

@ChristopherHX
Copy link
Contributor Author

  • Message for skipped steps

Isn't it partially here? you need to use -v aka verbose. I would recommend that your extension then filters debug/trace away by default, log level is a json property

logger.WithField("stepResult", stepResult.Outcome).Debugf("Skipping step '%s' due to '%s'", stepModel, ifExpression)

For jobs n/a I had fixed that the result is skipped, but I didn't think about the logger at that time.

  • stepResult for pre stage

Maybe debug this?, technically you should have this in the log
I mean

func runStepExecutor(step step, stage stepStage, executor common.Executor) common.Executor {
should be called for all stages and has the code for stepResult.

I noticed some log messages are not scoped in via this function in pre, like downloading the action aka (pre pre step, yes this get's weird, and it is)

@ChristopherHX
Copy link
Contributor Author

Job skipped log is also on debug level only and needs verbose flag

l.WithField("jobResult", "skipped").Debugf("Skipping job '%s' due to '%s'", job.Name, job.If.Value)

@SanjulaGanepola
Copy link

  • stepResult for pre stage

Maybe debug this?, technically you should have this in the log I mean

func runStepExecutor(step step, stage stepStage, executor common.Executor) common.Executor {

should be called for all stages and has the code for stepResult.
I noticed some log messages are not scoped in via this function in pre, like downloading the action aka (pre pre step, yes this get's weird, and it is)

@ChristopherHX I had a try with the following workflow:

name: Sample

on:
  push:
    branches: ["main"]

jobs:
  cli:
    name: CLI

    runs-on: ubuntu-latest

    steps:
      - name: Install Node
        uses: actions/setup-node@v4
        with:
          node-version: 18.x
          registry-url: 'https://registry.npmjs.org'

This has both a pre and post stage. I have attached the full json log below. There is a stepResult only for the step itself and the post stage. pre stage for some reason does not have it. In act, it would be best to always have a stepResult for every stage to ensure we can properly get back the status of it rather than having to assume it was successful.

output.log

@ChristopherHX
Copy link
Contributor Author

This has both a pre

No this nodejs action has no pre step, the logger might make you believe this is the case.

This action has a pre step: https://github.com/nektos/act-test-actions/tree/main/js-with-pre-and-post-step

Downloading the action is needed to check if it has a pre step, those log messages use the same log fields as the pre step log messages.

I wouldn't add this to this change either way

@SanjulaGanepola
Copy link

No this nodejs action has no pre step, the logger might make you believe this is the case.

Downloading the action is needed to check if it has a pre step, those log messages use the same log fields as the pre step log messages.

At the moment, is there a way to properly determine when there is a real pre step?

I feel that it would make more sense for there to be a different log type to avoid this issue. What do you think? And can this be implemented easily?

@ChristopherHX
Copy link
Contributor Author

is there a way to properly determine when there is a real pre step?

Checking for \u2B50 Run in message if type is pre then add it? May need to adapt the star emoji encoding for typescript.

Runner.Client logs everything as Main type and would be unaffected by such a hack.

I feel that it would make more sense for there to be a different log type to avoid this issue. What do you think? And can this be implemented easily?

..., moving downloading actions this into the setup job step makes more sense for me to align with actions/runner

@SanjulaGanepola
Copy link

moving downloading actions this into the setup job step makes more sense for me to align with actions/runner

I agree, this makes the most sense. pre step logs should strictly be for if the step truly does have a pre step.

…s-to-output-setup-clean-and-pre-stage-step-status
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for additional JSON fields to output setup, clean, and pre stage step status
2 participants