-
-
Notifications
You must be signed in to change notification settings - Fork 5
feat: allow for multiple channel inputs #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
Conversation
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.
Thanks for your contribution!
However, I would be very careful to determine if this additional feature makes sense.
If we add the ability to install multiple channels in the feature (as opposed to using lifecycle scripts) then users can take advantage of pre-build image caching!
In that case, I suspect you can use the onCreateCommand.
https://containers.dev/implementors/json_reference/#lifecycle-scripts
As a general matter, please also correct the followings:
- Bump the Feature's minor (or major) version.
- Update to pass tests.
Feel free to ask any questions!
|
Thank you @eitsupi!
I recently thought to too, but I've recently learned that lifecycle commands are not included in prebuilt Dev Container images. This is a bit confusing to me, because there is actually a Do you agree? Perhaps I'm missing something! If my description above is correct, then I think users would benefit from the ability to add multiple channels to the feature.
Do you have any preference here? I'd be inclined to make no breaking changes and release this as a minor version, personally. But I'm happy to implement whatever you think is best. Cheers! |
|
Thanks for explaining!
As you say, if it passes all existing tests, it is a good idea to bump the minor version. What I remembered about this was that some official Features use a separate environment variable to install multiple versions. The excellence of this approach is that it is clear which is the default version. |
Oh good idea, I like this approach better. I'll take a crack at that on Friday. |
fde0cf7 to
95426e9
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.
Could you change the version to 1.2.0?
| "version": "1.1.1", |
chore: cleanup whitespace test: add unit test for comma-separated channels test: correct unit test fix: dynamically find '$HOME' fix: variable expansion feat: add additionalChannels
Done! |
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 good, thanks!
Some minor comments.
Co-authored-by: eitsupi <[email protected]>
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 nice, thanks!
|
Thank you! |
If we add the ability to install multiple channels in the feature (as opposed to using lifecycle scripts) then users can take advantage of pre-build image caching!