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

Upgrade to spec 0.9 and libcnb 0.13.0 #157

Merged
merged 4 commits into from
Jul 6, 2023
Merged

Upgrade to spec 0.9 and libcnb 0.13.0 #157

merged 4 commits into from
Jul 6, 2023

Conversation

schneems
Copy link
Contributor

@schneems schneems commented Jun 29, 2023 •
edited by edmorley
Loading

Libcnb now targets spec version 0.9. The largest change is that bash processes no longer work for default processes. The workaround is to manually invoke bash -c (python example).

Bumps libcnb from 0.11.5 to 0.13.0.

updated-dependencies:

  • dependency-name: libcnb dependency-type: direct:production update-type: version-update:semver-minor ...

Closes #131.
GUS-W-13680510

@schneems
Copy link
Contributor Author

I think we need to upgrade the procfile buildpack to 0.9 spec too. Otherwise, I have no clue how the two will play together. I think this is good to go though.

@schneems schneems marked this pull request as ready for review June 29, 2023 16:54
@schneems schneems requested a review from a team as a code owner June 29, 2023 16:54
@schneems
Copy link
Contributor Author

schneems commented Jun 29, 2023 •
edited
Loading

Need to get cnb_shim inline with API spec 0.9 before procfile-cnb can be updated before this can be merged in heroku/buildpacks-procfile#151

@schneems schneems force-pushed the schneems/cargo/libcnb-0.13.0 branch from 18e7ff3 to 47fd65d Compare June 29, 2023 19:19
@edmorley
Copy link
Member

Need to get cnb_shim inline with API spec 0.9 before procfile-cnb can be updated before this can be merged in heroku/procfile-cnb#151

It's up to you, but I would say you could proceed with updating Ruby to the newer API even before cnb-shim is updated, since:

  • some other buildpacks have already updated
  • it only breaks people using cnb-shim with profile.d-creating legacy buildpacks (of which there aren't likely to be many at present)
  • we're still in a pre-beta phase where edge cases like combining with shimmed buildpacks are mostly out of scope IMO

Malax
Malax previously requested changes Jun 30, 2023
View reviewed changes
Copy link
Member

@Malax Malax left a comment

Choose a reason for hiding this comment

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

Request changes for the Display trait stuff. Feel free to ignore the quip about maybe not needing bashify at all - even tho I think that would be the better direction.

buildpacks/ruby/src/steps/get_default_process.rs Outdated Show resolved Hide resolved
buildpacks/ruby/src/steps/get_default_process.rs Outdated Show resolved Hide resolved
dependabot bot and others added 4 commits July 6, 2023 13:45
@dependabot @schneems
Bump libcnb from 0.11.5 to 0.13.0
e3480c3 
Bumps [libcnb](https://github.com/heroku/libcnb.rs) from 0.11.5 to 0.13.0.
- [Release notes](https://github.com/heroku/libcnb.rs/releases)
- [Changelog](https://github.com/heroku/libcnb.rs/blob/main/CHANGELOG.md)
- [Commits](heroku/libcnb.rs@v0.11.5...v0.13.0)

---
updated-dependencies:
- dependency-name: libcnb
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
@schneems
Update libcnb APIs
99378bb
@schneems
Add extremely explicit shell escaping
852548c
@schneems
Remove bashify experiment
d018ea6 
The prior implementation was overkill and the interface not really that helpful (except for forcing people to think about quoting and escaping).
@schneems schneems force-pushed the schneems/cargo/libcnb-0.13.0 branch from 47fd65d to d018ea6 Compare July 6, 2023 17:46
@schneems
Copy link
Contributor Author

schneems commented Jul 6, 2023

Manuel is out. I'm going to ask @edmorley for a review. I just implemented this suggestion #157 (comment). This is a two-way door, and I can always set up a refactoring session with Manuel when he comes back if there are further style considerations to explore.

@schneems schneems enabled auto-merge (squash) July 6, 2023 18:29
edmorley
edmorley approved these changes Jul 6, 2023
View reviewed changes
Copy link
Member

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

Looks great! :-)

@edmorley edmorley dismissed Malax’s stale review July 6, 2023 20:06

PR has been updated

@schneems schneems merged commit 974f624 into main Jul 6, 2023
@schneems schneems deleted the schneems/cargo/libcnb-0.13.0 branch July 6, 2023 20:06
edmorley
edmorley reviewed Jul 11, 2023
View reviewed changes
buildpacks/ruby/buildpack.toml
@@ -1,4 +1,4 @@
api = "0.8"
api = "0.9"
Copy link
Member

Choose a reason for hiding this comment

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

Ah this needs a CHANGELOG.md entry, since it affects the tool versions that work with the buildpack (old Pack CLI won't work with the buildpack etc)

@edmorley edmorley mentioned this pull request Mar 14, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edmorley edmorley edmorley approved these changes

@Malax Malax Malax left review comments

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

Successfully merging this pull request may close these issues.

Use direct: true when launching default process
3 participants
@schneems @edmorley @Malax