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

Add verbose logging to Analyze and Build phases #636

Merged
merged 3 commits into from May 26, 2020

Conversation

dfreilich
Copy link
Member

Closes #419

Signed-off-by: David Freilich dfreilich@vmware.com

@dfreilich dfreilich requested a review from a team as a code owner May 19, 2020 14:19
@codecov
Copy link

codecov bot commented May 19, 2020

Codecov Report

Merging #636 into release/0.11.0 will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                Coverage Diff                 @@
##           release/0.11.0     #636      +/-   ##
==================================================
+ Coverage           70.90%   70.94%   +0.04%     
==================================================
  Files                  68       68              
  Lines                4828     4834       +6     
==================================================
+ Hits                 3423     3429       +6     
  Misses               1078     1078              
  Partials              327      327              
Flag Coverage Δ
#os_linux 73.63% <100.00%> (+0.03%) ⬆️
#os_macos 69.73% <100.00%> (+0.03%) ⬆️
#os_windows 69.51% <100.00%> (+0.04%) ⬆️
#unit 70.94% <100.00%> (+0.04%) ⬆️

@natalieparellano
Copy link
Member

The file changes look good to me. I am getting an interesting problem when trying to run acceptance on your branch:

Released pack:

$  pack build localhost:5000/test-verbose -B cnbs/sample-builder:alpine -p apps/java-maven/ --publish
alpine: Pulling from cnbs/sample-builder
Digest: sha256:b12fef6427956b670aa3b9ff91daee19b927200dbf6bbb6171bf98be3360e8d7
Status: Image is up to date for cnbs/sample-builder:alpine
===> DETECTING
[detector] samples/java-maven 0.0.1
===> ANALYZING
[analyzer] Restoring metadata for "samples/java-maven:jdk" from app image
[analyzer] Restoring metadata for "samples/java-maven:maven_m2" from cache
===> RESTORING
...
Successfully built image localhost:5000/test-verbose

Dev pack:

$  ~/workspace/pack/out/pack build localhost:5000/test-verbose -B cnbs/sample-builder:alpine -p apps/java-maven/ --publish
alpine: Pulling from cnbs/sample-builder
Digest: sha256:b12fef6427956b670aa3b9ff91daee19b927200dbf6bbb6171bf98be3360e8d7
Status: Image is up to date for cnbs/sample-builder:alpine
===> CREATING
[creator] ---> DETECTING
[creator] samples/java-maven 0.0.1
[creator] ---> ANALYZING
[creator] ERROR: failed to get previous image: connect to repo store 'localhost:5000/test-verbose:latest': Get http://localhost:5000/v2/: dial tcp 127.0.0.1:5000: connect: connection refused
ERROR: failed with status code: 1

I don't think this change in behavior has anything to do with your changes, probably it's something else on master (you'll notice the creator is used instead of the 5 phases). Probably we want to understand this change because I don't think it is expected...

cc @jromero

@natalieparellano
Copy link
Member

It was the network mode :)

Dev pack:

$  ~/workspace/pack/out/pack build localhost:5000/test-verbose -B cnbs/sample-builder:alpine -p apps/java-maven/ --publish --network host -v
Pulling image index.docker.io/cnbs/sample-builder:alpine
alpine: Pulling from cnbs/sample-builder
Digest: sha256:b12fef6427956b670aa3b9ff91daee19b927200dbf6bbb6171bf98be3360e8d7
Status: Image is up to date for cnbs/sample-builder:alpine
Selected run image cnbs/sample-stack-run:alpine
Using build cache volume pack-cache-8640444c22b6.build
===> CREATING
[creator] ---> DETECTING
[creator] ======== Results ========
[creator] pass: samples/java-maven@0.0.1
[creator] Resolving plan... (try #1)
[creator] samples/java-maven 0.0.1
[creator] ---> ANALYZING
[creator] Analyzing image "localhost:5000/test-verbose@sha256:b77183f18a933d5b55bb0f0a5849487835432e0b15e0bab7ba196154d76d7d53"
[creator] Restoring metadata for "samples/java-maven:jdk" from app image
[creator] Writing layer metadata for "samples/java-maven:jdk"
[creator] Not restoring "samples/java-maven:jdk" from cache, marked as launch=true
[creator] Restoring metadata for "samples/java-maven:maven_m2" from cache
[creator] Writing layer metadata for "samples/java-maven:maven_m2"
[creator] ---> RESTORING

Release pack again because I forgot the -v in my comment above:

$  pack build localhost:5000/test-verbose -B cnbs/sample-builder:alpine -p apps/java-maven/ --publish --network host -v
Pulling image index.docker.io/cnbs/sample-builder:alpine
alpine: Pulling from cnbs/sample-builder
Digest: sha256:b12fef6427956b670aa3b9ff91daee19b927200dbf6bbb6171bf98be3360e8d7
Status: Image is up to date for cnbs/sample-builder:alpine
Selected run image cnbs/sample-stack-run:alpine
Using build cache volume pack-cache-8640444c22b6.build
===> DETECTING
[detector] ======== Results ========
[detector] pass: samples/java-maven@0.0.1
[detector] Resolving plan... (try #1)
[detector] samples/java-maven 0.0.1
===> ANALYZING
[analyzer] Restoring metadata for "samples/java-maven:jdk" from app image
[analyzer] Restoring metadata for "samples/java-maven:maven_m2" from cache
===> RESTORING
...

You'll notice that dev pack has more output so I think we can consider this one accepted :)

@natalieparellano
Copy link
Member

On second thought, I only verified this for analyze :( I am sure build is working, I just forgot to copy the output...

@jromero
Copy link
Member

jromero commented May 20, 2020

Thank you @natalieparellano for the user acceptance. Based on the changes, it would be safe to say that build would behave the same.

@jromero
Copy link
Member

jromero commented May 20, 2020

👍 Excellent work @dfreilich!

@natalieparellano
Copy link
Member

This is interesting - it appears from the test failures that lifecycle 0.6.1 doesn't support -log-level - but only for build? I wonder why that would be the case... it does seem supported on the latest lifecycle. Quickly glancing through the lifecycle code base, whatever changed (and the reason for it) isn't obvious to me - probably we want to check on this.

@dfreilich
Copy link
Member Author

@natalieparellano I saw that as well. I'm debating only adding it in for Analyze right now, to at least get it partly in, or diving in to lifecycle to figure out what's up with build

@jromero jromero changed the base branch from master to release/0.11.0 May 22, 2020 08:48
@jromero
Copy link
Member

jromero commented May 22, 2020

We obviously can't change a previously released implementation of the lifecycle. Could we add a check around the Platform API version in order to not set the flag for Platform API 0.2?

Copy link
Member

@jromero jromero left a comment

Choose a reason for hiding this comment

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

Given that this is going to a release branch, we'd want to take care of any compatibility issues in this PR before merging. Sorry @dfreilich.

@jromero jromero added this to the 0.11.0 milestone May 22, 2020
@dfreilich
Copy link
Member Author

@jromero Of course. Once the acceptance tests are fixed (as part of #644), I'll fix the PR

internal/build/phases.go Outdated Show resolved Hide resolved
Signed-off-by: David Freilich <dfreilich@vmware.com>
Signed-off-by: David Freilich <dfreilich@vmware.com>
Signed-off-by: David Freilich <dfreilich@vmware.com>
@dfreilich dfreilich merged commit 250679a into release/0.11.0 May 26, 2020
@dfreilich dfreilich deleted the verbose-logging branch May 26, 2020 17:32
@jromero jromero added the type/bug Issue that reports an unexpected behaviour. label May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Issue that reports an unexpected behaviour.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Verbose logging is not applied to analyze phase when publishing
3 participants