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.

Sign up for GitHub

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

Jump to bottom

Lifecycle image #1130

Merged
merged 7 commits into from
May 8, 2021
Merged

Lifecycle image #1130

merged 7 commits into from
May 8, 2021

Conversation

natalieparellano
Copy link
Member

Summary

Resolves #1105

@natalieparellano natalieparellano requested a review from a team as a code owner April 6, 2021 22:13
@github-actions github-actions bot added type/chore Issue that requests non-user facing changes. type/enhancement Issue that requests a new feature or improvement. labels Apr 6, 2021
@github-actions github-actions bot added this to the 0.19.0 milestone Apr 6, 2021
dfreilich
dfreilich reviewed Apr 9, 2021
View reviewed changes
Copy link
Member

@dfreilich dfreilich left a comment

Choose a reason for hiding this comment

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

Overall, this is really great! I'm split about what to do with the previous case though, I think we should support it...

acceptance/config/asset_manager.go Show resolved Hide resolved
acceptance/config/asset_manager.go Show resolved Hide resolved
@natalieparellano
Enable setting lifecycle image in pack acceptance tests
5274f2f 
Signed-off-by: Natalie Arellano <narellano@vmware.com>
@natalieparellano
Document LIFECYCLE_IMAGE
466fd8b 
Signed-off-by: Natalie Arellano <narellano@vmware.com>
jromero and others added 2 commits May 3, 2021 13:20
@jromero
Merge branch 'main' into lifecycle-image
116f500
@natalieparellano
Add PREVIOUS_LIFECYCLE_IMAGE
04c1ce6 
Signed-off-by: Natalie Arellano <narellano@vmware.com>
natalieparellano
natalieparellano commented May 7, 2021
View reviewed changes
DEVELOPMENT.md
Comment on lines -103 to -105
| PACK_PATH | Path to a `pack` executable. | A compiled version of the current branch |
| PREVIOUS_PACK_PATH | Path to a `pack` executable, used to test compatibility with n-1 version of `pack` | The most recent release from `pack`'s Github release |
| PREVIOUS_PACK_FIXTURES_PATH | Path to a set of fixtures, used to override the most up-to-date fixtures, in case of changed functionality | `acceptance/testdata/pack_previous_fixtures_overrides` |
Copy link
Member Author

Choose a reason for hiding this comment

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

Sorted alphabetically...

natalieparellano
natalieparellano commented May 7, 2021
View reviewed changes
acceptance/config/input_configuration_manager.go
Comment on lines -72 to -73
compilePackWithVersion: compilePackWithVersion,
githubToken: githubToken,
Copy link
Member Author

Choose a reason for hiding this comment

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

It seems fine to inline these.

natalieparellano
natalieparellano commented May 7, 2021
View reviewed changes
acceptance/config/input_configuration_manager.go
Comment on lines -16 to -21
envPackPath = "PACK_PATH"
envPreviousPackPath = "PREVIOUS_PACK_PATH"
envPreviousPackFixturesPath = "PREVIOUS_PACK_FIXTURES_PATH"
envLifecyclePath = "LIFECYCLE_PATH"
envPreviousLifecyclePath = "PREVIOUS_LIFECYCLE_PATH"
envGitHubToken = "GITHUB_TOKEN"
Copy link
Member Author

Choose a reason for hiding this comment

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

Sorted again...

@natalieparellano
Improve organization
2164781 
Signed-off-by: Natalie Arellano <narellano@vmware.com>
@natalieparellano
Further improvements
af2820f 
Always assign a valid lifecycle image if possible, so that we could clean it up

Signed-off-by: Natalie Arellano <narellano@vmware.com>
@natalieparellano
Copy link
Member Author

Alright I think I'm happy with it. Copying the results of some manual validation:

When lifecycle tgz contains a lifecycle that isn't on buildpacksio/lifecycle, it fails

LIFECYCLE_PATH=/Users/pivotal/workspace/lifecycle/out/lifecycle-v0.99.0+linux.x86-64.tgz make acceptance
> Running acceptance tests...
...
     |         lifecycle:
     |         |__ path: /Users/pivotal/workspace/lifecycle/out/lifecycle-v0.99.0+linux.x86-64.tgz
...
     |         ERROR: failed to build: fetching lifecycle image: image 'index.docker.io/buildpacksio/lifecycle:0.99.0' does not exist on the daemon: not found
...

When lifecycle tgz contains a lifecycle that isn't on buildpacksio/lifecycle, BUT you provide the lifecycle image, it succeeds

LIFECYCLE_IMAGE=buildpacksio/lifecycle:06bc728-dirty LIFECYCLE_PATH=/Users/pivotal/workspace/lifecycle/out/lifecycle-v0.99.0+linux.x86-64.tgz make acceptance
> Running acceptance tests...
...
     |         lifecycle:
     |         |__ path: /Users/pivotal/workspace/lifecycle/out/lifecycle-v0.99.0+linux.x86-64.tgz
...
PASS | github.com/buildpacks/pack/acceptance 27.596s

sonatype-lift[bot]
sonatype-lift bot reviewed May 7, 2021
View reviewed changes
acceptance/config/asset_manager.go
previousLifecycleImage := b.inputConfig.previousLifecycleImage

if previousLifecycleImage == "" {
previousLifecycleImage = fmt.Sprintf("%s:%s", config.DefaultLifecycleImageRepo, lifecycle.Descriptor().Info.Version)
Copy link

Choose a reason for hiding this comment

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

opt.semgrep.sprintf-host-port: Use net.JoinHostPort instead of fmt.Sprintf(config.DefaultLifecycleImageRepo, lifecycle.Descriptor().Info.Version). When using IPv6, JoinHostPort continues to operate properly.

(at-me in a reply with help or ignore)

Copy link
Member Author

Choose a reason for hiding this comment

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

@Muse-Dev ignore

Copy link

Choose a reason for hiding this comment

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

I've recorded this as ignored for this pull request. If you change your mind, just comment @muse-dev unignore.

@natalieparellano
Copy link
Member Author

natalieparellano commented May 7, 2021 •
edited
Loading

Same validation for previous:

When previous lifecycle tgz contains a lifecycle that isn't on buildpacksio/lifecycle, it fails

PREVIOUS_LIFECYCLE_PATH=/Users/pivotal/workspace/lifecycle/out/lifecycle-v0.99.0+linux.x86-64.tgz make acceptance-all
> Running acceptance tests...
...
     |         lifecycle:
     |         |__ path: /Users/pivotal/workspace/lifecycle/out/lifecycle-v0.99.0+linux.x86-64.tgz
...
     |         ERROR: failed to build: fetching lifecycle image: image 'index.docker.io/buildpacksio/lifecycle:0.99.0' does not exist on the daemon: not found

When previous lifecycle tgz contains a lifecycle that isn't on buildpacksio/lifecycle, BUT you provide the previous lifecycle image, it succeeds

PREVIOUS_LIFECYCLE_IMAGE=buildpacksio/lifecycle:06bc728-dirty PREVIOUS_LIFECYCLE_PATH=/Users/pivotal/workspace/lifecycle/out/lifecycle-v0.99.0+linux.x86-64.tgz make acceptance-all
> Running acceptance tests...
...
     |         lifecycle:
     |         |__ path: /Users/pivotal/workspace/lifecycle/out/lifecycle-v0.99.0+linux.x86-64.tgz
...
PASS | github.com/buildpacks/pack/acceptance 24.179s

@natalieparellano natalieparellano requested a review from dfreilich May 7, 2021 22:15
@jromero jromero merged commit ba48aa0 into main May 8, 2021
@jromero jromero deleted the lifecycle-image branch May 8, 2021 03:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sonatype-lift sonatype-lift[bot] sonatype-lift[bot] left review comments

@jromero jromero jromero approved these changes

@dfreilich dfreilich Awaiting requested review from dfreilich

Assignees
No one assigned
Labels
type/chore Issue that requests non-user facing changes. type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Milestone
0.19.0
Development

Successfully merging this pull request may close these issues.

lifecycle-image should be configurable in acceptance test suite
3 participants
@natalieparellano @jromero @dfreilich