-
Notifications
You must be signed in to change notification settings - Fork 212
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
Add cnb_app_lifecycle #929
Add cnb_app_lifecycle #929
Conversation
a3690ae
to
5c605c1
Compare
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.
Looks good!
Looks good to me. |
Thanks for doing this work. I am a bit newbie when it comes to CNB, so I ask you to please help me get educated along the way. I looked through the linked issues and was not sure if there is an overall acceptance-criteria for this feature. Something in the following format would be good to start with Scenario: describe scenario |
👋 Hi @c0d1ngm0nk3y, There is a little bit of awkwardness about this integration that I want to call out. One group is developing this repo, but a different group of people maintain diego release. |
Hi @ameowlia, we planned to bump the dependencies in the cnbapplifecycle repo directly, likely with dependabot or renovate. Unless of course we should not because the general practice in the WG is to manage Go dependencies in the Bosh release. We might want to discuss challenging the status quo then though ;) because we like to be able to build the Go binary without the context of the Diego release. Not sure what the long-term maintenance implications will be. We need to add CI to the repo (GitHub actions, if that's acceptable) and when we release, it of course needs to be bumped in the Diego release - we'll likely open PRs for this automatically. Also, we'll happily welcome contributors and/or maintainers to the new lifecycle repo. |
I'd probably go with Does that help @winkingturtle-vmw? More context can be found in buildpacks.io and paketo.io. |
Here are the four things that are top of mind for me and how they happen right now for other diego-release submodules. 1️⃣ Where will test be run 2️⃣ Bumping submodules in diego release 3️⃣ Go Updates 4️⃣ Dependency Bumps I think we should work to keep to this pattern as much as possible. We have our working group meeting in 5 minutes :) I'm sure we'll talk more about it there. |
If cnb-app-lifecycle doesn't need to import any of the go package from
We do this pattern already in at least nats-release. Would this be any easier for all of you and seem like a reasonable alternative? |
packages/cnb_app_lifecycle/packaging
Outdated
|
||
CGO_ENABLED=0 go build -o ${DEST}/ -ldflags "-s -w" -a -installsuffix static code.cloudfoundry.org/cnbapplifecycle/cmd/builder code.cloudfoundry.org/cnbapplifecycle/cmd/launcher | ||
|
||
GOOS=windows CGO_ENABLED=0 go build -o ${DEST}/builder.exe -ldflags "-s -w" -a -installsuffix static code.cloudfoundry.org/buildpackapplifecycle/builder |
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.
Has this been tested on Windows? I see that RFC have no mentions of OS level, so that's a good sign, but I am not yet educated enough to understand how this works on Windows?
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.
No we have not. We removed the Windows build for now. If there's interest for Windows within the community, we can bring this up again.
packages/cnb_app_lifecycle/spec
Outdated
|
||
files: | ||
- cnbapplifecycle/go.mod | ||
- cnbapplifecycle/go.sum |
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.
I see that cnbapplifecycle
is not vendoring it's dependencies. I believe this would fail when running in an internet-less env. We attempt to provide the compilations VMs everything it needs without the need to reach to the outside.
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.
Should be fixed now.
Sounds good. We will try this.
Understood.
But how to maintain the list of vendored folders in the
But how to synch on the dependencies? There could be conflicts, right?
We had a look, could you please check above question. |
This would be accomplished by running the linter. We update those depdendencies by running this daily job. |
@winkingturtle-vmw, @geofffranks , @ameowlia We tried to use ad
Open question: Will the automation take care of the vendoring and the |
@c0d1ngm0nk3y If you add _ "code.cloudfoundry.org/cnbapplifecycle/cmd/builder"
_ "code.cloudfoundry.org/cnbapplifecycle/cmd/launcher" to modules.go and then from
It will bring all of the required dependencies. You then have to add cnbapplifecyle to the list of packages to use gosub replacement. After that when you run the linter (linked above), it should populate the spec file with all of the required files to be used at bosh compile time. I hope that helps. |
3da10d5
to
6e5f360
Compare
We removed the Windows build and adjusted the PR according to the instructions. @winkingturtle-vmw could you please verify that we did not miss anything? |
src/code.cloudfoundry.org/go.mod
Outdated
@@ -1,6 +1,8 @@ | |||
module code.cloudfoundry.org | |||
|
|||
go 1.21.6 | |||
go 1.22 |
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.
I think this version got bumped because cnbapplifecycle is using 1.22. We always try to stay at least one minor version behind the tip. As of today, this would be go1.21. This is because we want to make sure all components have a chance to upgrade within the 6 month window for supporting the older version.
This might be okay for now, since go1.22 has been out for a while now, but in future, we wouldn't be able to consume latest version of this module if this version is bumped at every minor bump to be the latest. In other words, when go1.23 is out, we expect this to remain the same until go1.24 is out. As that point, it could be changed to go1.23 or remain unchanged (which is also the case for many of our modules)
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.
I think the bump for these submodules are not related to the cnbapplifecycle work. Can you go back for these submodules to be what is at develop
branch?
- routing-api
- routing-info
- garden
- guardian
- grootfs
|
||
pushd src/code.cloudfoundry.org | ||
|
||
CGO_ENABLED=0 go build -o ${DEST}/ -ldflags "-s -w" -a -installsuffix static code.cloudfoundry.org/cnbapplifecycle/cmd/builder code.cloudfoundry.org/cnbapplifecycle/cmd/launcher |
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.
code.cloudfoundry.org/cnbapplifecylce is missing a LICENSE. This would get flagged during the OSL process, can we make sure that there is a LICENSE for this repo?
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.
added in cloudfoundry/cnbapplifecycle#15
63a8964
to
6462dd8
Compare
bfcf07d
to
d79fb4d
Compare
@modulo11 this looks good now. We now need someone to deploy this release and verify it's functionality. We have a story for this PR. |
3c8eab0
to
cd90bac
Compare
@winkingturtle-vmw what is the status of this - and what's the plan? I.e. what kind of test do you plan to do - just whether this cleanly installs with Bosh I guess? |
cd90bac
to
a8076f0
Compare
@loewenstein There is a story in our backlog to deploy this PR and verify it's functionality. We are also going to turn on CATs tests in our pipeline for this feature. Additionally, I don't see any information around what CNBs are supported in the first iteration of this feature and plans around supporting other CNBs in the future. It would be helpful for the community to know the roadmap for this feature (maybe as a markdown in cnbapplifecycle repo or a series of issues) since it's getting added for every deploy of CF. We don't have anyone assigned to the story yet, but it's in our backlog and we will get to it. |
Thanks @winkingturtle-vmw for the response. I'd argue that this PRs functionality is pretty limited as it only adds another lifecycle binary to the file server. Regarding the community questions: all Cloud Native Buildpacks that can tolerate cflinuxfs4 as rootfs are going to be supported. Once the functionality is actually available, i.e. all remaining PRs merged, it's probably time to add cloud native buildpack support to CF Docs. TL;DR the near term roadmap for Buildpacks is documented in the RFCs, user facing documentation blocked on the open PRs and I would hope to get this PR merged soon and hopefully disentangled from an e2e test run that includes ~10 PRs with more than half of them actually already merged anyway. |
@winkingturtle-vmw @ameowlia Any chance to get this in? It's one of the last pieces of the puzzle - only CF CLI and cf-deployment left and there's also RFC31 System CNBs already merged and waiting for implementation. |
1c6acc8
to
1c94c90
Compare
@loewenstein-sap to update on status. I tried to deploy the code, but have been getting go mod compilation errors. Trying to understand what is happening.. |
1c94c90
to
d93ac48
Compare
Hi @mariash, could you please provide details about what kind of error you are seeing. |
@loewenstein-sap this is the error I am getting:
|
The steps to reproduce:
|
@aminjam noticed an issue with the spec file missing modules.txt file, once I added it deploy succeeded |
d93ac48
to
aa0096f
Compare
@mariash Thanks. With that input, I was able to:
|
@c0d1ngm0nk3y Hi, could you please rebase onto the latest code. Our CI is green now and we paused it so that nothing gets merged until your PR is merged. |
Co-authored-by: Pavel Busko <pavel.busko@sap.com> Co-authored-by: Nicolas Bender <nicolas.bender@sap.com> Co-authored-by: Johannes Dillmann <j.dillmann@sap.com>
Co-authored-by: Johannes Dillmann <j.dillmann@sap.com>
aa0096f
to
0e61a43
Compare
@mariash Is there a diego-release release planned already that will contain this? It's going to be needed to merge cloudfoundry/cf-deployment#1178. |
Summary
This adds another app lifecycle called
cnbapplifecycle
to thediego-release
to allow apps to build and start with cloud native buildpacks.cnbapplifecycle
- https://github.com/cloudfoundry/cnbapplifecycleBackward Compatibility
Breaking Change? No