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.

Sign up for GitHub

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

Jump to bottom

(IMP) bug fix: missing devcontainer-feature properties are now processed in features #126

Merged
merged 7 commits into from
Aug 17, 2022
Merged

(IMP) bug fix: missing devcontainer-feature properties are now processed in features #126

merged 7 commits into from
Aug 17, 2022

Conversation

samruddhikhandale
Copy link
Member

@samruddhikhandale samruddhikhandale commented Aug 13, 2022 •
edited
Loading

@samruddhikhandale samruddhikhandale requested a review from a team as a code owner August 13, 2022 01:33
@Chuxel
Copy link
Member

Chuxel commented Aug 15, 2022

Just to double check - I assume this will also cover v1 features with extensions and settings at the top level for a feature?

@samruddhikhandale
Copy link
Member Author

Just to double check - I assume this will also cover v1 features with extensions and settings at the top level for a feature?

@Chuxel yes, tested it locally, they are covered. 👍

@Chuxel Chuxel requested a review from joshspicer August 15, 2022 17:55
joshspicer
joshspicer previously approved these changes Aug 15, 2022
View reviewed changes
Copy link
Member

@joshspicer joshspicer left a comment

Choose a reason for hiding this comment

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

Would be great to also get @chrmarti & @edgonmsft to take a peek, but i think this is 👍

@Chuxel Chuxel requested a review from alexdima August 16, 2022 13:41
@Chuxel
Copy link
Member

Chuxel commented Aug 16, 2022

@chrmarti is OOF this week. Adding @alexdima who is back tomorrow.

@joshspicer
Copy link
Member

I think it would be worth adding tests here as well https://github.com/devcontainers/cli/blob/main/src/test/container-features/generateFeaturesConfig.test.ts

@samruddhikhandale samruddhikhandale mentioned this pull request Aug 16, 2022
Closed
joshspicer
joshspicer previously approved these changes Aug 16, 2022
View reviewed changes
@joshspicer
Copy link
Member

One more quick note - with devcontainers/features#97 we will lose the regression testing for top-level extensions/settings, etc..

Would be cool to add one more test (maybe with local features) to test that top level vscode attributes continue to work indefinitely.

alexdima
alexdima approved these changes Aug 17, 2022
View reviewed changes
Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

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

Looks good!

@joshspicer joshspicer merged commit b1b41eb into main Aug 17, 2022
@joshspicer joshspicer deleted the samruddhikhandale/fix-customizations-bug branch August 17, 2022 14:20
joshspicer pushed a commit that referenced this pull request Aug 17, 2022
@samruddhikhandale @joshspicer
(IMP) bug fix: missing devcontainer-feature properties are now proces…
0fa9777 
…sed in features (#126)

* fix bug: customizations

* fix lint

* address comments

* add tests

* address comments

* address comments: add tests

* add more tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexdima alexdima alexdima approved these changes

@joshspicer joshspicer joshspicer left review comments

@chrmarti chrmarti Awaiting requested review from chrmarti

@edgonmsft edgonmsft Awaiting requested review from edgonmsft

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

"customizations" (and legacy extension, settings properties) not processed in features
4 participants
@samruddhikhandale @Chuxel @joshspicer @alexdima