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

Humanize v-e-c Task logs #1769

Merged
merged 4 commits into from
Jul 26, 2024
Merged

Conversation

zregvart
Copy link
Member

The top of the logfile for a TaskRun of verify-enterprise-contract Task should be the text (human readable) report. Following it are the YAML and JSON logs and preceded by a break ----- DEBUG OUTPUT ----- are the debug logs and version information.

This is to make the TaskRun's logs easier to understand by humans.

Also adds the --logfile parameter to redirect the logging to a file.

Reference: EC-564

Copy link

codecov bot commented Jul 19, 2024

Codecov Report

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

Project coverage is 80.74%. Comparing base (b66e1a9) to head (751f2e5).
Report is 3 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1769      +/-   ##
==========================================
- Coverage   80.76%   80.74%   -0.03%     
==========================================
  Files          68       68              
  Lines        4918     4954      +36     
==========================================
+ Hits         3972     4000      +28     
- Misses        946      954       +8     
Flag Coverage Δ
generative 80.74% <67.74%> (-0.03%) ⬇️
integration 80.74% <67.74%> (-0.03%) ⬇️
unit 80.74% <67.74%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
cmd/validate/definition.go 97.53% <100.00%> (ø)
cmd/validate/image.go 94.95% <100.00%> (+0.03%) ⬆️
internal/definition/report.go 87.80% <75.00%> (-2.20%) ⬇️
cmd/validate/input.go 47.44% <75.00%> (+1.58%) ⬆️
internal/applicationsnapshot/report.go 72.04% <70.00%> (+4.06%) ⬆️
internal/format/target.go 82.35% <85.71%> (-4.02%) ⬇️
internal/input/report.go 70.27% <0.00%> (-4.02%) ⬇️
internal/logging/logging.go 25.35% <0.00%> (-1.93%) ⬇️

@@ -160,6 +155,7 @@ spec:

- name: validate
image: quay.io/enterprise-contract/ec-cli:snapshot
onError: continue
Copy link
Member

Choose a reason for hiding this comment

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

Will the Task still fail if there's an error, e.g. invalid policy config is provided? I think so.

Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for adding this line? Maybe a comment is worthwhile.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a comment, ec could fail, for example because bad input was provided and the rest of the steps would be skipped, including the output of debug logs so we would loose valuable clues as to how to debug the issue

cmd/root/root_cmd.go Outdated Show resolved Hide resolved
image: quay.io/enterprise-contract/ec-cli:snapshot
command: [ec]
args:
- version
Copy link
Member

@simonbaird simonbaird Jul 22, 2024

Choose a reason for hiding this comment

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

Maybe some extra debug:

      - name: images
        ...
          echo "$(params.IMAGES)"

      - name: policy-source
        ...
           echo "($params.POLICY_CONFIGURATION)"

      - name: policy
         ...
            jq .policy < "$(params.HOMEDIR)/report-json.json"

Copy link
Member

Choose a reason for hiding this comment

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

FYI, every step we add means a new container is created.

@zregvart zregvart force-pushed the issue/EC-564 branch 2 times, most recently from 6314f3a to e2ec9f2 Compare July 24, 2024 13:39
@simonbaird
Copy link
Member

/retest

zregvart added 4 commits July 25, 2024 10:34
This is useful so we can separate in contexts where stderr/stdout is not
as clear, e.g. when running as a Tekton Task.

Reference: EC-564
Makes showing of successes flag a field of the Report structure.

Reference: EC-564
In addition to the global flag `--show-successes` that influences all
outputs and this adds `--show-successes` equivalent per output format.
This allows the full report with successes to be present in JSON or YAML
outputs while the output in text format can be outputted without
successes.

This is done by appending options in URL query format to the output
format following a question mark, e.g.
`--output text=output.txt?show-successes=false`.

Reference: EC-564
The top of the logfile for a TaskRun of verify-enterprise-contract Task
should be the text (human readable) report. Following it are the YAML
and JSON logs and preceded by a break `----- DEBUG OUTPUT -----` are
the debug logs and version information.
This is to make the TaskRun's logs easier to understand by humans.

Reference: EC-564
@@ -159,6 +160,7 @@ func NewReport(snapshot string, components []Component, policy policy.Policy, da
Data: data,
PolicyInput: policyInput,
EffectiveTime: policy.EffectiveTime().UTC(),
ShowSuccesses: showSuccesses,
Copy link
Member

Choose a reason for hiding this comment

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

As discussed, should also have this one based on the --info flag:

+                      ShowInfo: showInfo,

Could be done later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at the code that might be a bit more work, so I'll file a separate story about this

@zregvart zregvart merged commit 85ae44e into enterprise-contract:main Jul 26, 2024
14 of 15 checks passed
@zregvart zregvart deleted the issue/EC-564 branch July 26, 2024 07:32
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.

3 participants