8000 Add LaunchBuilder by Malax · Pull Request #487 · heroku/libcnb.rs · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add LaunchBuilder #487

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
Merged
merged 3 commits into from
Aug 17, 2022
Merged

Add LaunchBuilder #487

merged 3 commits into from
Aug 17, 2022

Conversation

Malax
Copy link
Member
@Malax Malax commented Aug 17, 2022

This PR originally started as a fix for #485. As it turns out, adding slices to Launch was already possible, albeit more complicated as it should be since there were no builder functions available for slices.

This PR adds a dedicated LaunchBuilder that allows setting all fields for Launch, removes the builder-style functions from Launch as they were inconsistent with the rest of libcnb.rs anyways. I still want to tackle #166, but this isn't the PR for that.

I also added some docs for Slice that clarify what paths actually is (and renamed it). I opted to not implement a newtype for Go standard library path globs for now as we can spend our time better in other areas. I created an issue for it as #486.

Changelog

  • Replace builder style functions from Launch with a dedicated LaunchBuilder to be more consistent with other builders in the library. Additionally, all fields of Launch can now be modified via the builder pattern.
  • Rename paths field in launch::Slice to path_globs and add docs to make it clearer that these strings are Go standard library globs.

Closes #485, GUS-W-11486542

@Malax Malax marked this pull request as ready for review August 17, 2022 10:10
@Malax Malax requested a review from a team as a code owner August 17, 2022 10:10
@Malax Malax mentioned this pull request Aug 17, 2022
Closed
@Malax
Copy link
Member Author
Malax commented Aug 17, 2022 •
edited
Loading

Currently looking into a fix for the integration test failures that are triggered by us having multiple heroku/procfile buildpacks in the builder used for the integration tests. These failures are very likely not related to this PR.

Edit: Solved via 0e4279c

@Malax Malax force-pushed the malax/launch-builder branch from 89cf784 to 89cc7f8 Compare August 17, 2022 12:35
@Malax Malax force-pushed the malax/launch-builder branch from 89cc7f8 to 0e4279c Compare August 17, 2022 13:04
edmorley
edmorley approved these changes Aug 17, 2022
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.

Thank you!

@Malax Malax merged commit d0a35f7 into main Aug 17, 2022
@Malax Malax deleted the malax/launch-builder branch August 17, 2022 16:32
@schneems
Copy link
Contributor

A little late on the draw, but I reviewed this. Looks good.

I see that the slice and slices functions take something that implements Into Slice. I don't see where we're implementing that anywhere other than the default that any struct can be made into itself (though perhaps I missed it).

Do we have examples of building a slice or should we just manually create the struct for now

libcnb.rs/libcnb-data/src/launch.rs

Lines 225 to 232 in d0a35f7

pub struct Slice {
/// Path globs for this slice.
///
/// These globs need to follow the pattern syntax defined in the [Go standard library](https://golang.org/pkg/path/filepath/#Match)
/// and only match files/directories inside the application directory.
#[serde(rename = "paths")]
pub path_globs: Vec<String>,
}
?

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

Assignees

@Malax Malax

Labels
breaking change libcnb-data
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for slice layers
3 participants
@Malax @schneems @edmorley
0