Skip to content

Conversation

@tomkennedy513
Copy link

Thanks for contributing to cloud_controller_ng. To speed up the process of reviewing your pull request please provide us with:

  • A short explanation of the proposed change:

  • An explanation of the use cases your change solves

  • Links to any other associated PRs

  • I have reviewed the contributing guide

  • I have viewed, signed, and submitted the Contributor License Agreement

  • I have made this pull request to the main branch

  • I have run all the unit tests using bundle exec rake

  • I have run CF Acceptance Tests

tomkennedy513 and others added 2 commits April 25, 2025 11:31
- This is a follow on to cloudfoundry#3898 which only supports uploading CNBs as zip or tgz whereas they are released in cnb format

Signed-off-by: Tom Kennedy <tom.kennedy@broadcom.com>
@tomkennedy513
Copy link
Author

@pbusko going to check with @sethboyles to make sure the validate buildpack commit is done before marking ready

@tomkennedy513 tomkennedy513 marked this pull request as ready for review April 25, 2025 16:31
@tomkennedy513
Copy link
Author

@pbusko this can be merged, we will likely follow up with another pr making a couple other changes

Comment on lines +22 to +23

dataset = dataset.where(lifecycle: message.lifecycle) if message.requested?(:lifecycle)
Copy link

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)?

Copy link
Author

@tomkennedy513 tomkennedy513 Apr 30, 2025

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.

Copy link

@pbusko pbusko Apr 30, 2025

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.

Copy link
Author

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.

Copy link

@sethboyles sethboyles Apr 30, 2025

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:

Screenshot 2025-04-30 at 9 54 50 AM

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.

Copy link
Author

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.

Copy link

Choose a reason for hiding this comment

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

Currently the cnb lifecycle works only with custom buildpacks, so it is not an issue. Apps can be created with either but the cf buildpacks is still reliably returns buildpacks for the buildpack lifecycle. I do see filtering as a necessity.

perhaps the default filtering should be a configuration option?

This sounds like a valid approach, but with the filtering enabled by default.

Copy link

@sethboyles sethboyles May 2, 2025

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.

Copy link

@Gerg Gerg May 2, 2025

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:

  1. Returning additional resources from an endpoint or adding fields to a resource is not a breaking change.
  2. The CLI is not the only client of the API, so we don't want to over-fit the API design to the CLI.
  3. I'm not aware of any prior art for hiding resources by default on v3 CAPI. It would be confusing for API users & client authors to have an endpoint that behaves inconsistently with other endpoints.
  4. I would not consider showing somewhat confusing output in old versions of the CLI to be a CLI breaking change either, especially since it would only affect environments that have CNBs installed.
  5. If CLI users run into issues, they can always upgrade their CLI version.

Some mitigations:

  1. The CNB buildpack names could reflect their lifecycle (e.g. ruby_cnb), so it would be clear to users what they are, even if their CLI version doesn't display the "lifecycle".
  2. Operators can configure min CLI versions in their environments to prompt users to upgrade: https://github.com/cloudfoundry/capi-release/blob/a36693b72164c93a57f01957f278bc35a124a0cd/jobs/cloud_controller_ng/spec#L416-L424

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.

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.

@sethboyles sethboyles force-pushed the add-cnb-file-support branch from cbefbae to 2cfc863 Compare May 2, 2025 22:49
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.

4 participants