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.

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

Features preamble: Add warnings for Feature renames & deprecation #366

Merged
merged 8 commits into from
Jan 5, 2023

Conversation

samruddhikhandale
Copy link
Member

Deprecation msg
image

Renaming msg
image

@samruddhikhandale samruddhikhandale requested a review from a team as a code owner January 4, 2023 23:46
joshspicer
joshspicer previously approved these changes Jan 4, 2023
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.

Nice polish!

jkeech
jkeech previously approved these changes Jan 5, 2023
Copy link
Contributor

@jkeech jkeech left a comment

Choose a reason for hiding this comment

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

Thanks for applying the changes. I left one more comment about continuing to use sh instead of bash, but otherwise looks good.

src/spec-configuration/containerFeaturesConfiguration.ts Outdated Show resolved Hide resolved
jkeech
jkeech previously approved these changes Jan 5, 2023
Copy link
Contributor

@jkeech jkeech left a comment

Choose a reason for hiding this comment

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

LGTM. Left a minor suggestion to simplify the generated script a bit, particularly in the "no warning" case.

src/spec-configuration/containerFeaturesConfiguration.ts Outdated Show resolved Hide resolved
@samruddhikhandale samruddhikhandale merged commit d1c0dac into main Jan 5, 2023
@samruddhikhandale samruddhikhandale deleted the samruddhikhandale/legacyIds-info branch January 5, 2023 23:08
}

if (feature?.legacyIds && feature.legacyIds.length > 0 && feature.currentId && feature.id !== feature.currentId) {
warningHeader += `(!) WARNING: This feature has been renamed. Please update the reference in devcontainer.json to "${escapeQuotesForShell(feature.currentId)}".`;
Copy link

Choose a reason for hiding this comment

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

Wouldn't it be better to end with \n same as line 261?

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.

4 participants