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

Improve detector log output #975

Merged
merged 4 commits into from
Dec 14, 2022

Conversation

natalieparellano
Copy link
Member

When detect fails, print output as info level

Resolves #967

@natalieparellano natalieparellano requested a review from a team as a code owner December 9, 2022 17:20
@codecov
Copy link

codecov bot commented Dec 9, 2022

Codecov Report

Merging #975 (edf11b3) into main (da5da9b) will increase coverage by 0.28%.
The diff coverage is n/a.

❗ Current head edf11b3 differs from pull request most recent head 3f4cfa5. Consider uploading reports for the commit 3f4cfa5 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #975      +/-   ##
==========================================
+ Coverage   64.44%   64.71%   +0.28%     
==========================================
  Files          86       86              
  Lines        6171     6154      -17     
==========================================
+ Hits         3976     3982       +6     
+ Misses       1820     1796      -24     
- Partials      375      376       +1     
Flag Coverage Δ
os_linux 64.17% <ø> (+0.28%) ⬆️
os_windows 59.02% <ø> (+0.28%) ⬆️
unit 64.17% <ø> (+0.28%) ⬆️

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

@natalieparellano natalieparellano marked this pull request as draft December 9, 2022 17:48
@natalieparellano natalieparellano force-pushed the feature/fail-detect-log-info-level branch from ed6582a to c45e4cc Compare December 9, 2022 18:03
log/multi_logger.go Outdated Show resolved Hide resolved
@natalieparellano natalieparellano marked this pull request as ready for review December 9, 2022 18:10
@@ -106,7 +106,7 @@ func testDetector(t *testing.T, when spec.G, it spec.S) {
"docker",
"run",
"--rm",
"--env", "CNB_ORDER_PATH=/cnb/orders/empty_order.toml",
Copy link
Member Author

Choose a reason for hiding this comment

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

Empty order produces no output, so it's not a good test

@natalieparellano
Copy link
Member Author

@jabrown85 it would be awesome to get your input here (as the requestor of #283) and also because I'm a bit unsure of some of the choices I made. The implementation ended up being a tad more complicated than I initially expected. Feedback is very welcome!

@natalieparellano natalieparellano added this to the lifecycle 0.16.0 milestone Dec 9, 2022
detector.go Outdated Show resolved Hide resolved
This eliminates the need for a "multi logger"

Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Copy link
Member Author

Oof, the linux ARM runner needs to be re-created

detector.go Outdated
Runs: &sync.Map{},
MemHandler: memHandler,
Copy link
Contributor

@jabrown85 jabrown85 Dec 13, 2022

Choose a reason for hiding this comment

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

I think we could straight up hide MemHandler and let it get built and stored in Detector. It isn't something a caller needs to send in. More of an implementation detail IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right... updated in 0479907

Signed-off-by: Natalie Arellano <[email protected]>
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano natalieparellano force-pushed the feature/fail-detect-log-info-level branch from 0505d95 to 3f4cfa5 Compare December 14, 2022 15:38
Copy link
Contributor

@jabrown85 jabrown85 left a comment

Choose a reason for hiding this comment

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

CI issue appears transient - I like the changes!

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.

Include full lifecycle log output on failed builds to improve debugging
2 participants