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

Use CNB_PLATFORM_API to adjust what default process to show #796

Merged
merged 4 commits into from
Aug 12, 2020
Merged

Use CNB_PLATFORM_API to adjust what default process to show #796

merged 4 commits into from
Aug 12, 2020

Conversation

dfreilich
Copy link
Member

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

Summary

This is an attempt to change the default process type Pack displays for images created with a lifecycle implementing Platform API 0.4.

Output

Before

After

Documentation

  • Should this change be documented?
    • No

Related

Resolves #789

@dfreilich dfreilich requested a review from a team as a code owner August 11, 2020 21:49
@dfreilich
Copy link
Member Author

@jromero I'm not sure whether or not this solves the problem. I based this off this line in the RFC:

We can use CNB_PLATFORM_API (API "modes") to toggle between the current and new launcher process resolution flow. CNB_PLATFORM_API will be set in the exported image to the platform API provided at build time, but can be explicitly overridden at runtime.

The lifecycle release (https://github.com/buildpacks/lifecycle/releases/tag/v0.9.0) and code seems to show that they set it.

@dfreilich
Use CNB_PLATFORM_API to adjust what default process to show
59b23d7 
Signed-off-by: David Freilich <dfreilich@vmware.com>
@codecov
Copy link

codecov bot commented Aug 11, 2020 •
edited

Codecov Report

Merging #796 into main will increase coverage by 0.07%.
The diff coverage is 92.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #796      +/-   ##
==========================================
+ Coverage   77.62%   77.69%   +0.07%     
==========================================
  Files          78       78              
  Lines        4521     4544      +23     
==========================================
+ Hits         3509     3530      +21     
- Misses        702      703       +1     
- Partials      310      311       +1
Flag Coverage Δ
#os_linux 77.69% <92.31%> (+0.12%) ⬆️
#os_macos 73.77% <92.31%> (+0.09%) ⬆️
#os_windows 100.00% <ø> (ø)
#unit 77.69% <92.31%> (+0.07%) ⬆️

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

@jromero
Copy link
Member

jromero commented Aug 11, 2020

@dfreilich This makes a lot of sense. I think this is the right solution.

jromero
jromero approved these changes Aug 11, 2020
View reviewed changes
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.

Thanks for the quick turnaround. I left a minor nitpick. Feel free to merge as-is and I can address it via #794 since I'll likely be touching this area.

inspect_image.go Outdated Show resolved Hide resolved
natalieparellano
natalieparellano requested changes Aug 11, 2020
View reviewed changes
Copy link
Member

@natalieparellano natalieparellano left a comment

Choose a reason for hiding this comment

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

Requesting changes because from my understanding, the parsing for platform api >= 0.4 is not correct. But perhaps I am confused. Feel free to address my comments and merge if I've got it wrong.

@jromero jromero added this to the 0.13.0 milestone Aug 12, 2020
@jromero jromero added the type/enhancement Issue that requests a new feature or improvement. label Aug 12, 2020
@dfreilich
Move strings to consts
94f8af2 
* Add bad env test

Signed-off-by: David Freilich <dfreilich@vmware.com>
dfreilich
dfreilich commented Aug 12, 2020
View reviewed changes
inspect_image.go Outdated Show resolved Hide resolved
@jromero jromero mentioned this pull request Aug 12, 2020
Merged
@dfreilich
Use ENTRYPOINT to find correct process type
9c7cee7 
Signed-off-by: David Freilich <dfreilich@vmware.com>
natalieparellano
natalieparellano reviewed Aug 12, 2020
View reviewed changes
inspect_image.go Outdated Show resolved Hide resolved
natalieparellano
natalieparellano reviewed Aug 12, 2020
View reviewed changes
inspect_image.go Outdated Show resolved Hide resolved
natalieparellano
natalieparellano reviewed Aug 12, 2020
View reviewed changes
Copy link
Member

@natalieparellano natalieparellano left a comment

Choose a reason for hiding this comment

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

@dfreilich other than the Windows modifications, this looks good to me!

@dfreilich
Copy link
Member Author

Added support for inspecting Windows images, even on other machines

jromero
jromero suggested changes Aug 12, 2020
View reviewed changes
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.

This is soooo close. Just added one comment about making slight changes to the tests.

inspect_image_test.go Outdated Show resolved Hide resolved
@dfreilich
Add support for inspecting Windows images
63dcea9 
* Ideally, you should be able to inspect Windows images regardless of which OS machine you are using, so I didn't use filepath.Join() for creating the paths

Signed-off-by: David Freilich <dfreilich@vmware.com>
@dfreilich dfreilich requested a review from jromero August 12, 2020 19:25
@jromero jromero merged commit 5e9a21f into main Aug 12, 2020
@jromero jromero deleted the fix/789-cnb-process-type branch August 12, 2020 21:49
@jromero jromero changed the title Use CNB_PLATFORM_API to adjust what default process to show Use CNB_PLATFORM_API to adjust what default process to show Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jromero jromero jromero approved these changes

@natalieparellano natalieparellano Awaiting requested review from natalieparellano

Assignees
No one assigned
Labels
type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Milestone
0.13.0
Development

Successfully merging this pull request may close these issues.

Omit CNB_PROCESS_TYPE when listing processes
3 participants
@dfreilich @jromero @natalieparellano