forked from cloudfoundry/cloud_controller_ng
-
Notifications
You must be signed in to change notification settings - Fork 1
Add cnb file support #374
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
Closed
tomkennedy513
wants to merge
6
commits into
sap-contributions:cnb-system-buildpacks
from
tomkennedy513:add-cnb-file-support
Closed
Add cnb file support #374
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
e2d2b3f
Add support for uploading buildpacks packaged as cnb (tar) files
tomkennedy513 07231d1
Validate buildpack filetype based on lifecycle
sethboyles 2cfc863
Remove default buildpacks filter and change default sort to 'lifecycle'
sethboyles 4ec2e9e
Allow specifying buildpacks with CNBs
sethboyles 0eb9b20
Rubocop fixes
sethboyles 860074a
Support .cnb, .tgz, and .tar.gz as part of install_buildpacks
sethboyles File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how the list will look when called with an older version of CLI (by doing
cf buildpacks)?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the cli explicitly sends an order_by of position, the list will be mixed, but we felt that it was better to have that behavior because the default filter was breaking the update-buildpack and delete-buildpack commands if the buildpack was using a non default lifecycle because the buildpack could not be found without the lifecycle query in the api call.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But since all buildpacks endpoints now have the lifecycle parameter, the mutating commands have to specify it. We'll introduce these flags to the CLI in the following PR after this one is merged. I believe that filtering is a must for backward compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then old clis would break if the default lifecycle is changed. Or if someone uploads a build pack with a non default lifecycle and older cli can’t delete it. In my opinion the buildpack list being jumbled is not a breaking change as much as commands breaking.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pbusko can you explain what this will break? It is just confusing? We already have precendence for intermingled positions with stacks:
It's not ideal for lifecycles, since that's not in the table yet,
but at least stack will be blank, so there is a clue that these buildpacks are different.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm creating a new app I could use either lifecycle and I may not know that I need to look for multiple lifecycles. We could make multiple calls but that seems unnecessary when we could just make the filtering configurable and leave it up to the operator whether they should be returned in a single table or not. I could also see an issue if another lifecycle is added one day and the cli would need to be updated to add another call for filtering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the
cnblifecycle works only with custom buildpacks, so it is not an issue. Apps can be created with either but thecf buildpacksis still reliably returns buildpacks for thebuildpacklifecycle. I do see filtering as a necessity.This sounds like a valid approach, but with the filtering enabled by default.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now we don't have default filtering anywhere in the API and I don't think we should include it. It's inherently confusing.
Additionally, developers can still push with CNB buildpacks on the old CLI if they are using manifests. It will be confusing if they can't see those buildpacks at all.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, hiding resources on the API by default is too cautious; buildpacks of all lifecycles should be returned by default.
Some thoughts:
Some mitigations:
ruby_cnb), so it would be clear to users what they are, even if their CLI version doesn't display the "lifecycle".I personally wouldn't add a new manifest feature flag to add opt-in filtering behavior, but I don't oppose it, if you feel strongly that it's important to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pbusko We are planning to merge in your PR + this PR tomorrow. When releasing, we'll be sure to include a release note indicating the behavior of the buildpacks endpoint if CNBs are added. If you want to include a configuration flag to add default filtering, that's something we can add at a later date.