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

azd env set add warning #4547

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

azd env set add warning #4547

wants to merge 8 commits into from

Conversation

NanaXiong00
Copy link

@NanaXiong00 NanaXiong00 commented Nov 13, 2024

Fixes issue #289.

azd env set adds warning about case sensitivity: warn if the same value is present but in mixed case.
As shown in the figure below:
image

@rajeshkamal5050 for notification.

Copy link
Contributor

@wbreza wbreza 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 - but please update warning style to be consistent with UX guidelines before merging.

cli/azd/cmd/env.go Outdated Show resolved Hide resolved
conflictKeysStr := strings.Join(conflictKeys, " and ")
fmt.Print(
output.WithWarningFormat(
"WARNING: The environment variable %s already exists in the .env file with a different case.\n",
Copy link
Contributor

@weikanglim weikanglim Nov 14, 2024

Choose a reason for hiding this comment

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

Let's hold off on merging, I'd like to continue with the requirements discussion here before we finalize the changes.

Copy link
Member

Choose a reason for hiding this comment

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

@weikanglim ping.

Copy link
Contributor

Choose a reason for hiding this comment

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

@vhvb1989 pushed a change, does this look good to you?

Copy link
Member

Choose a reason for hiding this comment

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

merge merge merge

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just me but the word "instead" is throwing me off a bit since it's printing the same name used in the original command. I think it makes more sense to either omit "instead" or change the phrasing to "Did you mean to set 'foo' instead":

image

Any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

It definitely should be reworded as did you mean to set 'foo' instead?

@vhvb1989 The multiple insensitive cases is still a weird situation :D it almost clearly signals user intention of wanting to set multiple different casings. Still feels as-if the confirmation would be a better approach

or we can just stop printing warning messages for n>1

@microsoft-github-policy-service microsoft-github-policy-service bot added the no-recent-activity identity issues with no activity label Jan 17, 2025
Copy link
Contributor

Hi @NanaXiong00. Thank you for your interest in helping to improve the Azure Developer CLI experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

@v-xuto v-xuto removed the no-recent-activity identity issues with no activity label Jan 17, 2025
@weikanglim weikanglim requested a review from JeffreyCA as a code owner January 23, 2025 20:58
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.

6 participants