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.

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

feat(manager): Cloud Native Buildpacks project descriptor manager #30799

Merged

Conversation

pbusko
Copy link
Contributor

@pbusko pbusko commented Aug 15, 2024

Changes

  • Adds support for Cloud Native Buildpacks project descriptor manager
  • Updates docker references to builder and/or buildpacks in the project.toml file

Context

project.toml file is a convenient way to keep Cloud Native Buildpacks configuration, when using pack CLI to produce OCI images. The project descriptor file contains references to OCI images used during build, which could be updated by Renovate

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@CLAassistant
Copy link

CLAassistant commented Aug 15, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ pbusko
❌ loewenstein-sap
You have signed the CLA already but the status is still pending? Let us recheck it.

@pbusko pbusko force-pushed the feat/cnb-project-descriptor-support branch from 2ee3e78 to f66c732 Compare August 15, 2024 08:50
lib/constants/category.ts Outdated Show resolved Hide resolved
lib/modules/manager/cnb-project-descriptor/extract.ts Outdated Show resolved Hide resolved
lib/modules/manager/cnb-project-descriptor/index.ts Outdated Show resolved Hide resolved
@rarkins
Copy link
Collaborator

rarkins commented Aug 15, 2024

Can we find a simpler name for this manager, maybe even "buildpacks"?

@pbusko
Copy link
Contributor Author

pbusko commented Aug 15, 2024

Can we find a simpler name for this manager, maybe even "buildpacks"?

sounds good to me

@pbusko pbusko requested a review from rarkins August 15, 2024 10:47
lib/modules/manager/buildpacks/extract.spec.ts Outdated Show resolved Hide resolved
lib/modules/manager/buildpacks/extract.ts Show resolved Hide resolved
lib/modules/manager/buildpacks/extract.ts Outdated Show resolved Hide resolved
lib/modules/manager/buildpacks/extract.ts Outdated Show resolved Hide resolved
lib/modules/manager/buildpacks/index.ts Show resolved Hide resolved
lib/modules/manager/buildpacks/__fixtures__/project.toml Outdated Show resolved Hide resolved
@pbusko pbusko changed the title feat: Cloud Native Buildpacks project descriptor manager feat(manager): Cloud Native Buildpacks project descriptor manager Aug 16, 2024
@c0d1ngm0nk3y
Copy link

Can we find a simpler name for this manager, maybe even "buildpacks"?

How about cnb? buildpacks is maybe confusing - there are several versions of buildpacks. The abbreviation cnb (cloud native buildpacks) should be known to anyone project.toml.

@rarkins
Copy link
Collaborator

rarkins commented Aug 16, 2024

I'm pretty sure the CNF now have a trademark in buildpacks so this will be the only valid usage of that name

@Shegox
Copy link
Contributor

Shegox commented Aug 16, 2024

I'm pretty sure the CNF now have a trademark in buildpacks so this will be the only valid usage of that name

They do: https://www.linuxfoundation.org/legal/trademarks

  • Buildpacks®
  • Buildpacks.io™
  • Buildpacks.IO and Design (color)

@c0d1ngm0nk3y
Copy link

I'm pretty sure the CNF now have a trademark in buildpacks so this will be the only valid usage of that name

I don't think that matters since the v2 buildpacks are in CNCF, aren't they?

But probably the name dosn't really matter anyway. The end user will not have any contact unless a project.toml is present and for internal use there is still the readme.md if in doubt.

lib/modules/manager/cnb/readme.md Outdated Show resolved Hide resolved
lib/modules/manager/cnb/readme.md Outdated Show resolved Hide resolved
lib/modules/manager/cnb/readme.md Outdated Show resolved Hide resolved
lib/modules/manager/cnb/readme.md Outdated Show resolved Hide resolved
@HonkingGoose
Copy link
Collaborator

Thanks for applying my suggestions. The next time, please batch apply the suggestions. Here's how:

  1. Go to your PR's Files changed tab.
  2. Select the Add suggestion to batch button for each suggestion.
  3. After batching all suggestions: commit.

Read GitHub Docs, incorporating feedback into your pull request to learn more.

@pbusko
Copy link
Contributor Author

pbusko commented Aug 16, 2024

@HonkingGoose thank you, will definitely do it in the future. The contribution guide states:

PRs will always be squashed by us when we merge your work. Commit as many times as you need in your pull request branch.

So I thought that the number of commits/batching would make no difference

@HonkingGoose
Copy link
Collaborator

@HonkingGoose thank you, will definitely do it in the future. The contribution guide states:

PRs will always be squashed by us when we merge your work. Commit as many times as you need in your pull request branch.

So I thought that the number of commits/batching would make no difference

Yeah, that's a partial instruction! We really mean:

  • Commit as often as you want
  • But batch review suggestions (to limit GitHub notification noise)

I'm improving the instructions with:

Copy link
Collaborator

@HonkingGoose HonkingGoose left a comment

Choose a reason for hiding this comment

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

One small thing, after that the docs look good to me.

I'll let a maintainer check for technical accuracy.

lib/modules/manager/cnb/readme.md Outdated Show resolved Hide resolved
lib/modules/manager/cnb/extract.spec.ts Outdated Show resolved Hide resolved
lib/modules/manager/cnb/extract.ts Outdated Show resolved Hide resolved
lib/modules/manager/cnb/extract.ts Outdated Show resolved Hide resolved
lib/modules/manager/cnb/extract.ts Outdated Show resolved Hide resolved
lib/modules/manager/buildpacks/__fixtures__/project.toml Outdated Show resolved Hide resolved
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

otherwise LGTM

lib/modules/manager/cnb/extract.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@rarkins rarkins left a comment

Choose a reason for hiding this comment

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

Please update from main branch then run pnpm prettier-fix

@pbusko pbusko requested a review from rarkins August 21, 2024 08:00
@rarkins rarkins requested a review from viceice August 21, 2024 11:33
@pbusko pbusko requested a review from viceice August 23, 2024 06:42
@rarkins
Copy link
Collaborator

rarkins commented Nov 12, 2024

That's what my colleagues tried to hint at here and here.

I don't think they were hinting at catastrophic backtracking, and even if they were they should have spoken clearly, not hinted.

They wrote:

that regex would be rather complex itself

Sounds to me like "it's hard", not "it's impossible without catastrophic backtracking"

We try the regex approach to identify a docker ref. If we miss it, it will be ignored although it would work.

Sounds to me like "we might get it wrong"

@loewenstein-sap
Copy link
Contributor

I don't think they were hinting at catastrophic backtracking, and even if they were they should have spoken clearly, not hinted.

They did not hint at "catastrophic backtracking" per se, but they were clearly speaking out for not going down the road of a complex regex.

Should we revert to the pre-regex state or what should be done on this PR to get it merged?

@pbusko
Copy link
Contributor Author

pbusko commented Nov 13, 2024

the regex has been optimized, there should be no catastrophic backtracking issues anymore

@rarkins
Copy link
Collaborator

rarkins commented Nov 13, 2024

@pbusko
Copy link
Contributor Author

pbusko commented Nov 13, 2024

Does https://chatgpt.com/share/e/67349544-aa74-800b-9647-9bac228f5ba4 help?

unfortunately I have no access to the link, but the problem should be solved (at least GHAS marked all issues as solved)

@rarkins
Copy link
Collaborator

rarkins commented Nov 13, 2024

Does chatgpt.com/share/e/67349544-aa74-800b-9647-9bac228f5ba4 help?

unfortunately I have no access to the link, but the problem should be solved (at least GHAS marked all issues as solved)

Sorry, I thought "share link" would be public. Maybe not

@rarkins rarkins added this pull request to the merge queue Nov 21, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 21, 2024
@rarkins rarkins added this pull request to the merge queue Nov 21, 2024
Merged via the queue into renovatebot:main with commit bbedb2d Nov 21, 2024
38 of 39 checks passed
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 39.25.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@pbusko pbusko deleted the feat/cnb-project-descriptor-support branch November 21, 2024 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto:no-mentions Don't cause unnecessary notifications
Projects
None yet
Development

Successfully merging this pull request may close these issues.