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

Include component dependencies in OCI artifact #2893

Merged

Conversation

itowlson
Copy link
Contributor

Fixes #2891.

@fibonacci1729 @vdice this is rather "works on my machine" at the moment; do you have any thoughts on how to rassle it into a test harness?

Copy link
Contributor

@fibonacci1729 fibonacci1729 left a comment

Choose a reason for hiding this comment

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

This looks good to me! I'm also not sure about how to add a test harness or how best to test this. Nonetheless, manually tested so it at least works on 2 machines! 🪅

Copy link
Member

@vdice vdice left a comment

Choose a reason for hiding this comment

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

Re: testing, we did have a unit test around assemble_layers which is now commented-out from some iteration of factors re-factoring. Adding a case for component deps there could've made sense -- but we'd need to uncomment/update that test first.

Meanwhile, I don't yet see equivalent unit tests for loader.

Do we think we're at a state where we also might want some sort of integration test with a local registry, etc?

All of which is to say, if @fibonacci1729 agrees, I'd say we're fine to get this in and then address some tech/testing debt in follow-up(s).

@itowlson itowlson force-pushed the bundle-dependencies-in-registry-apps branch from 0d7553e to 23c4f7f Compare October 23, 2024 21:21
@itowlson
Copy link
Contributor Author

@vdice Integration tests with a local registry sound like a good longer terms plan. It gets gnarly for e2e though because a real test requires you to reference a component from Registry A, push the application to Registry B, and then turn off Registry A...

In the meantime I've fixed the existing assemble_layers test and added a case for a dependency (not very visible because of the comment diff, sorry). Thanks for highlighting this.

@vdice
Copy link
Member

vdice commented Oct 23, 2024

In the meantime I've fixed the existing assemble_layers test and added a case for a dependency

🎉 Thank you!

@itowlson itowlson merged commit 18c06af into fermyon:main Oct 23, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Raise error when trying to push app with local dependencies
3 participants