-
Notifications
You must be signed in to change notification settings - Fork 333
add features test ... command
#11
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's some work underway that @chrmarti and I have been discussing to enable dev container images to be build with inline cache data. This is to enable dev container image caching to work with container-features in environments such as CI where intermediate images from previous builds aren't available. To enable this, we plan to combine the container features into a single, multi-stage build step with main image build, so I'm not sure how well that would work with this command? An alternative might be a helper function that takes the feature details and generates the |
|
Having a
I don't think it'll really change this? Right now this is essentially just doing a I'd love your feedback @chrmarti on the best way to "hook into" building a devcontainer from a CLI command (you can see i'm using the |
|
Ah, I'd misunderstood the appraoch - sorry! |
| const output = `${prefix} ${msg}\n`; | ||
|
|
||
| if (options?.stderr) { | ||
| process.stderr.write(chalk.red(output)); |
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.
Let's use a Log object like other commands.
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.
I'm finding that the Log is printing out a lot more information than I want (timestamp, header, etc..). Also, I can't easily color the message.
What reason do you have for using Log in this case?
| ...args | ||
| ] | ||
| }; | ||
| const result = await doExec(execArgs); |
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.
You could probably inline launch and use the parameter objects that gets from resolve to call runRemoteCommand directly. Maybe check if that makes things clearer.
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.
I tried to do this and found that I was duplicating a lot of code from doExec, and it was overall not working as well ($PATH wasn't accessible). I very likely was doing something wrong, but leaving this here for now. Happy to chat more about this though
(and would be nice to find a more standardized pattern. to invoke one CLI command from another)
1419216 to
03a2cae
Compare
|
eg output, testing two features |
2d54320 to
7be3288
Compare
* support fetching v2 features from github repo * update test
Changes for Features V2
| @@ -0,0 +1,118 @@ | |||
| export const staticProvisionParams = { | |||
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.
I would love if we refactored the code a bit to make the provision and exec command easier to call internally. I couldn't seem to find a good blend of "easy to use" without moving or copying a bunch of code :)
c76c5a1 to
52de545
Compare
52de545 to
761a232
Compare
|
Reapplied on main as a single commit in #81. |
https://github.com/github/codespaces/issues/8002
Adds
devcontainer features test(Folks with visibility can check out https://github.com/devcontainers/features/tree/main/test and https://github.com/devcontainers/features/tree/main/test-scenarios)
Example usage