Skip to content

Add --additional-features option #171

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

Merged
merged 2 commits into from
Oct 3, 2022

Conversation

stuartleeks
Copy link
Collaborator

Initial work to enable additional features to be specified via the CLI

@stuartleeks stuartleeks force-pushed the sl/additional-features branch 3 times, most recently from b8b3d6d to 3d0317e Compare September 17, 2022 20:09
@stuartleeks stuartleeks changed the title WIP Add --additional-features option Add --additional-features option Sep 17, 2022
@stuartleeks stuartleeks marked this pull request as ready for review September 17, 2022 20:16
@stuartleeks stuartleeks requested a review from a team as a code owner September 17, 2022 20:16
@stuartleeks stuartleeks force-pushed the sl/additional-features branch from 3d0317e to 0e006ea Compare September 30, 2022 14:36
Copy link
Contributor

@chrmarti chrmarti left a comment

Choose a reason for hiding this comment

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

Looks great. Left two comments for change / discussion. Thanks!

@stuartleeks stuartleeks force-pushed the sl/additional-features branch 2 times, most recently from fd35f1a to 7f2c89d Compare October 3, 2022 14:19
@stuartleeks
Copy link
Collaborator Author

@chrmarti - I've pushed an update that hopefully addresses your feedback

@chrmarti
Copy link
Contributor

chrmarti commented Oct 3, 2022

@chrmarti - I've pushed an update that hopefully addresses your feedback

@stuartleeks Still seeing the additional clause in readConfiguration. I think we do without it to avoid reading the entire features config when the container already exists:
image

Make additionalFeatures parameter required
Avoid requerying features for existing container
@stuartleeks stuartleeks force-pushed the sl/additional-features branch from 7f2c89d to 70adff6 Compare October 3, 2022 14:59
@stuartleeks
Copy link
Collaborator Author

Oops! I'm not sure what happened there - sorry!

I have reapplied the change to remove additionalFeatures from the needsFeaturesConfig expression.

Copy link
Contributor

@chrmarti chrmarti left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@stuartleeks stuartleeks merged commit 6cb3ce6 into devcontainers:main Oct 3, 2022
@stuartleeks stuartleeks deleted the sl/additional-features branch October 3, 2022 15:47
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.

2 participants