-
Notifications
You must be signed in to change notification settings - Fork 19
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
Component filtering #197
Component filtering #197
Conversation
Signed-off-by: Kate Goldenring <[email protected]>
1006722
to
b9abe16
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me - I'm curious if there's anyway to expose some of this logic around component retention in Spin so that the spinkube doesn't need to copy the logic.
@rylev i agree that this is entirely duplicated code. I brought up this topic in the Spin PR but we punted not knowing the best place for it: fermyon/spin#2826 (comment). That being said, I'll look at moving it to a crate |
@Mossaka @devigned @jsturtevant @radu-matei would love your thoughts on this. |
I don't have a strong opinion about this particular change. It makes sense and is minimally invasive. The point about duplicating code is probably going to continue without a better option. I can't recall the context of the conversation, but at some point recently I believe @Mossaka and I were discussing if many of these changes could be avoided by exposing the I think extending the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
One concern that i have is that this seems like a lot of sharing code between here and fermyon/spin#2826 and this smell tells me that there is an opportunity to extract them to a library and share between these two projects.
@Mossaka, @rylev brought up the same point that i addressed here: #197 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@devigned, I always thought changing/duplicating |
I felt the same way about duplication of the
I think these are some interesting cases which deserve some consideration. |
@devigned have you looked at the component dependencies work @fibonacci1729 has brought into Spin? It enables referencing upstream components in your @Mossaka for continuing this discussion in an issue, I feel like Spin may be the best place for that since we are talking about OCI artifact contents |
Merging it rn. |
oh oops, I forgot to mention, can you add this change to the CHANGELOG, starting with Unreleased section? 🙏 |
Is this something we will do for all PRs? I am not sure I understand making that a manual process rather than generating a CHANGELOG on release or having someone do it all at once. Is this a best practice you've seen in other projects? I can see how it is nice to be able to see what the changes are on main easily. |
Spin CLI added support for
spin up --component-id "foo" --component-id "bar"
to run a subset of components of a Spin app in fermyon/spin#2826.This brings that functionality to the shim assuming that the components to support are supplied in
SPIN_COMPONENTS_TO_RETAIN
environment variable of a Pod as a comma separated list (SPIN_COMPONENTS_TO_RETAIN="foo,bar"
).In the Spin Operator, we can add a field to the SpinApp CR spec called
components
that lists which components of an app to retain in a pod.This is on top of @rylev's changes #189. The additive changes are in 1006722