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

Make compatible with libcnb 0.18.0 (Buildpack API 0.10) #199

Closed
edmorley opened this issue Feb 12, 2024 · 3 comments
Closed

Make compatible with libcnb 0.18.0 (Buildpack API 0.10) #199

edmorley opened this issue Feb 12, 2024 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@edmorley
Copy link
Member

edmorley commented Feb 12, 2024

The PR to update this repository to libcnb 0.18.0 (which includes the switch to Buildpack API 0.10 and thus targets) is currently failing:
#195

...with this error:

---- buildpacks::test::test_read_image_repository_metadata_empty stdout ----
thread 'buildpacks::test::test_read_image_repository_metadata_empty' panicked at src/buildpacks.rs:117:80:
called `Result::unwrap()` on an `Err` value: Error { inner: Error { inner: TomlError { message: "data did not match any variant of untagged enum BuildpackDescriptor", original: None, keys: [], span: None } } }

---- buildpacks::test::test_read_image_repository_metadata stdout ----
thread 'buildpacks::test::test_read_image_repository_metadata' panicked at src/buildpacks.rs:97:80:
called `Result::unwrap()` on an `Err` value: Error { inner: Error { inner: TomlError { message: "data did not match any variant of untagged enum BuildpackDescriptor", original: None, keys: [], span: None } } }

See:
https://github.com/heroku/languages-github-actions/actions/runs/7873635043/job/21481438671?pr=195#step:5:64

The tests in question are here:

#[test]
fn test_read_image_repository_metadata() {
let data = r#"
api = "0.9"
[buildpack]
id = "foo/bar"
version = "0.0.1"
[[stacks]]
id = "*"
[metadata.release.image]
repository = "repository value"
"#;
let buildpack_descriptor = toml::from_str::<BuildpackDescriptor>(data).unwrap();
assert_eq!(
read_image_repository_metadata(&buildpack_descriptor),
Some("repository value".to_string())
);
}
#[test]
fn test_read_image_repository_metadata_empty() {
let data = r#"
api = "0.9"
[buildpack]
id = "foo/bar"
version = "0.0.1"
[[stacks]]
id = "*"
"#;
let buildpack_descriptor = toml::from_str::<BuildpackDescriptor>(data).unwrap();
assert_eq!(read_image_repository_metadata(&buildpack_descriptor), None);
}

The reason they are failing is since the test uses a buildpack.toml that contains the no longer supported [[stacks]] TOML table, and the implementation in this repository uses the libcnb BuildpackDescriptor type to deserialise the buildpack.toml.

One way to fix the failures would be to update the Dependabot PR with changes to the test fixtures to use [[targets]] instead of [[stacks]].

However, once the changes are released, this will break all CNBs using the automation in this repository until they update to newer libcnb themselves (which is something they won't be able to do immediately, since they'll need to make their own changes for compatibility with stacks->targets). We could of course pre-emptively pin every buildpack's usage of the shared workflow to @<version> and then switch back to @<latest> later, but (a) that seems like a bit of a hassle, (b) it means some buildpacks might be stuck using an older version of the automation for some time, until they can be updated (eg the procfile CNB which is stuck on old libcnb).

Another option would be to make the buildpack.toml parser used in this repository more lax - and have a custom type in this repo that only contains the fields actually used by the automation? That way it will work with both new and libcnb versions, giving more flexibility both now and in the future.

Much longer term I think this issue highlights the need to push more of the Rust code in this repo upstream into eg libcnb-cargo, so we don't have such a hard coupling in APIs between this repo and libcnb's.

@Malax What are your thoughts?

@edmorley edmorley added the enhancement New feature or request label Feb 12, 2024
@Malax
Copy link
Member

Malax commented Feb 13, 2024

While I was reading your comment, I mentally prepared my answer which is pretty much what you wrote in the end:

Much longer term I think this issue highlights the need to push more of the Rust code in this repo upstream into eg libcnb-cargo, so we don't have such a hard coupling in APIs between this repo and libcnb's.

I'm not sure how "long term" this really is though. This isn't the first time this coupling causes issues. IMO, we should prioritize making this change.

To unblock both this repository and buildpacks that will update to 0.10 buildpack API, relaxing the requirements for buildpack.toml in this repository makes sense to me.

@edmorley
Copy link
Member Author

The failures here will be resolved by heroku/libcnb.rs#789 once released in a new libcnb version.

@edmorley edmorley self-assigned this Feb 26, 2024
@edmorley
Copy link
Member Author

This was resolved in #200, and released in:
https://github.com/heroku/languages-github-actions/releases/tag/v0.5.0

Note: That there is still a compatibility issue between Pack CLI and using [[targets]], see:
buildpacks/pack#2047

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants