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

[RFC0028] Implement Cloud Native Buildpacks lifecycle #3778

Merged
merged 10 commits into from
Jun 19, 2024
Merged

[RFC0028] Implement Cloud Native Buildpacks lifecycle #3778

merged 10 commits into from
Jun 19, 2024

Conversation

pbusko
Copy link
Contributor

@pbusko pbusko commented May 2, 2024

  • A short explanation of the proposed change:

Implementation of the RFC-0028

  • An explanation of the use cases your change solves

This PR adds third lifecycle option: cnb. This lifecycle type utilises Cloud Native Buildpacks to produce droplets.

  • Links to any other associated PRs

https://github.com/cloudfoundry/community/blob/main/toc/rfc/rfc-0028-cnb-lifecycle.md

  • 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

@pbusko pbusko mentioned this pull request May 2, 2024
Merged
3 tasks
@pbusko pbusko force-pushed the cnb-lifecycle branch 3 times, most recently from d0778ec to 44f73ad Compare May 7, 2024 11:54
This was referenced May 13, 2024
Merged
Merged
Merged
@pbusko pbusko force-pushed the cnb-lifecycle branch 2 times, most recently from 4cdf7ca to 62e6911 Compare May 14, 2024 08:45
@pbusko pbusko marked this pull request as ready for review May 14, 2024 08:46
johha
johha requested changes May 31, 2024
View reviewed changes
Copy link
Contributor

@johha johha left a comment

Choose a reason for hiding this comment

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

Still need to go through the unit tests. So far my main concerns are:

  • Do we need the buildpack_url in the cnb lifecycle model? Might be needed for revisions
  • Code duplication in diego part. I'd prefer to use inheritance as much as possible

I'll look into why the backward compatibility unit tests are failing for rotate_database_key_spec.rb. I assume because the table is generated after the test runs.

lib/utils/uri_utils.rb Outdated Show resolved Hide resolved
lib/utils/uri_utils.rb Show resolved Hide resolved
app/actions/droplet_create.rb Outdated Show resolved Hide resolved
app/messages/app_manifest_message.rb Outdated Show resolved Hide resolved
app/models/runtime/app_model.rb Outdated Show resolved Hide resolved
lib/cloud_controller/diego/cnb/staging_action_builder.rb Outdated Show resolved Hide resolved
lib/cloud_controller/diego/cnb/staging_completion_handler.rb Show resolved Hide resolved
lib/cloud_controller/diego/cnb/task_action_builder.rb Outdated Show resolved Hide resolved
lib/cloud_controller/diego/lifecycles/app_cnb_lifecycle.rb Outdated Show resolved Hide resolved
lib/cloud_controller/diego/lifecycles/cnb_lifecycle.rb Outdated Show resolved Hide resolved
@johha
Copy link
Contributor

johha commented Jun 4, 2024

Related to my previous comment about the code duplication in the diego part it might be worth it to check if the tests could be also simplified, e.g. by using shared examples

@nicolasbender nicolasbender mentioned this pull request Jun 6, 2024
Merged
5 tasks
@c0d1ngm0nk3y c0d1ngm0nk3y force-pushed the cnb-lifecycle branch 4 times, most recently from 4d1d1ba to 3376aa2 Compare June 11, 2024 08:41
@c0d1ngm0nk3y
Copy link
Contributor

Still need to go through the unit tests. So far my main concerns are:

  • Do we need the buildpack_url in the cnb lifecycle model? Might be needed for revisions

As discussed in person. We don't need the buildpack_url to be encrypted, but we reuse the buildpack_lifecycle_buildpacks relation.

  • Code duplication in diego part. I'd prefer to use inheritance as much as possible

We removed the code duplication where we saw beneficial. Could you have another look.

I'll look into why the backward compatibility unit tests are failing for rotate_database_key_spec.rb. I assume because the table is generated after the test runs.

The tests were green in the last commit, but after fixing a rubocop offense, the tests fail again. Any idea?

@modulo11 modulo11 force-pushed the cnb-lifecycle branch 2 times, most recently from 2e5994d to 4251f09 Compare June 12, 2024 06:55
@c0d1ngm0nk3y
Copy link
Contributor

@johha We think we have addressed all review comments. Please let us know if we missed something.

@loewenstein-sap loewenstein-sap mentioned this pull request Jun 12, 2024
Open
johha
johha reviewed Jun 12, 2024
View reviewed changes
Copy link
Contributor

@johha johha left a comment

Choose a reason for hiding this comment

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

I modified the db migration for better robustness

@c0d1ngm0nk3y c0d1ngm0nk3y force-pushed the cnb-lifecycle branch 2 times, most recently from 05404ba to 2b9d8a6 Compare June 14, 2024 12:17
johha
johha requested changes Jun 18, 2024
View reviewed changes
Copy link
Contributor

@johha johha left a comment

Choose a reason for hiding this comment

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

small migration optimization

db/migrations/20240514113500_create_cnb_lifecycle_data.rb Outdated Show resolved Hide resolved
spec/unit/lib/cloud_controller/errands/rotate_database_key_spec.rb Outdated Show resolved Hide resolved
pbusko and others added 5 commits June 18, 2024 13:14
@pbusko @c0d1ngm0nk3y
@modulo11 @nicolasbender
Make docker URI parser reusable
ac918b5 
Co-authored-by: Ralf Pannemans <ralf.pannemans@sap.com>
Co-authored-by: Johannes Dillmann <j.dillmann@sap.com>
Co-authored-by: Nicolas Bender <nicolas.bender@sap.com>
@pbusko @c0d1ngm0nk3y
@modulo11 @nicolasbender
Add cnb_lifecycle_data model
84e1b64 
Co-authored-by: Ralf Pannemans <ralf.pannemans@sap.com>
Co-authored-by: Johannes Dillmann <j.dillmann@sap.com>
Co-authored-by: Nicolas Bender <nicolas.bender@sap.com>
Co-authored-by: Pavel Busko <pavel.busko@sap.com>
@pbusko @c0d1ngm0nk3y
@modulo11 @nicolasbender
Add diego_cnb feature flag
4851792 
Co-authored-by: Ralf Pannemans <ralf.pannemans@sap.com>
Co-authored-by: Johannes Dillmann <j.dillmann@sap.com>
Co-authored-by: Nicolas Bender <nicolas.bender@sap.com>
@pbusko @c0d1ngm0nk3y
@modulo11 @nicolasbender
Add CNB specific error codes for staging
48a75f9 
Co-authored-by: Ralf Pannemans <ralf.pannemans@sap.com>
Co-authored-by: Johannes Dillmann <j.dillmann@sap.com>
Co-authored-by: Nicolas Bender <nicolas.bender@sap.com>
@pbusko @c0d1ngm0nk3y
@modulo11 @nicolasbender
Add Cloud Native Buildpacks lifecycle
b2366d7 
Co-authored-by: Ralf Pannemans <ralf.pannemans@sap.com>
Co-authored-by: Johannes Dillmann <j.dillmann@sap.com>
Co-authored-by: Nicolas Bender <nicolas.bender@sap.com>
c0d1ngm0nk3y and others added 2 commits June 18, 2024 13:25
@c0d1ngm0nk3y @nicolasbender
@modulo11 @johha @pbusko
Address review feedback, e.g. reduce code duplication
86c5b94 
Co-authored-by: Nicolas Bender <nicolas.bender@sap.com>
Co-authored-by: Johannes Dillmann <j.dillmann@sap.com>
Co-authored-by: Johannes Haass <johannes.haass@sap.com>
Co-authored-by: Pavel Busko <pavel.busko@sap.com>
@c0d1ngm0nk3y
20240514113500_create_cnb_lifecycle_data.rb
da18b18
@c0d1ngm0nk3y
Copy link
Contributor

@johha I had to do small changes to your suggestion of the migration file. Could you have a look here at da18b18ee4e46888? I think it still fits what you intended to change, right?

@c0d1ngm0nk3y
Copy link
Contributor 

PG::UndefinedTable: ERROR:  relation \"schema_info\" does not exist

@johha Any idea why the tests fail again?

@c0d1ngm0nk3y
Copy link
Contributor

@johha I did more testing of the revisions feature and the app behaves the same with both lifecycles. There were some glitches, but those could be seen for both lifecycles. So the expectation that it should be the same was not wrong after all. It just another way in producing the droplet. Hence I have enabled revisions again with dc01106.

@c0d1ngm0nk3y
Copy link
Contributor

All checks have passed

👍

@pbusko pbusko requested a review from johha June 19, 2024 10:19
johha
johha approved these changes Jun 19, 2024
View reviewed changes
Copy link
Contributor

@johha johha left a comment

Choose a reason for hiding this comment

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

lgtm 👍 🤞

@johha johha merged commit daffedc into cloudfoundry:main Jun 19, 2024
14 checks passed
@pbusko pbusko deleted the cnb-lifecycle branch June 19, 2024 10:32
@c0d1ngm0nk3y
Copy link
Contributor

acceptance were successful with this version deployed 👍

@johha johha mentioned this pull request Jun 19, 2024
Merged
@modulo11 modulo11 mentioned this pull request Jun 20, 2024
Merged
5 tasks
@johha johha mentioned this pull request Jul 17, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@c0d1ngm0nk3y c0d1ngm0nk3y c0d1ngm0nk3y left review comments

@johha johha johha approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pbusko @johha @c0d1ngm0nk3y