Skip to content

[AppConfig]Fix pagination and globalization bugs. #11522

Merged
Juliehzl merged 7 commits intoAzure:devfrom
shenmuxiaosen:user/shuai/pagination
Dec 12, 2019
Merged

[AppConfig]Fix pagination and globalization bugs. #11522
Juliehzl merged 7 commits intoAzure:devfrom
shenmuxiaosen:user/shuai/pagination

Conversation

@shenmuxiaosen
Copy link
Copy Markdown
Contributor

@shenmuxiaosen shenmuxiaosen commented Dec 10, 2019

  • Fix bug when appending api-version to request url. The existing solution doesn't work with pagination.
  • For globalization, we should support showing languages besides English as our backend service support unicode.

This checklist is used to make sure that common guidelines for a pull request are followed.

  • The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).

  • I adhere to the Command Guidelines.

@shenmuxiaosen
Copy link
Copy Markdown
Contributor Author

shenmuxiaosen commented Dec 10, 2019

Please add @avanigupta as reviewer. Thanks.

@shenmuxiaosen shenmuxiaosen changed the title Fix appconfig pagination and globalization bugs. [AppConfig]Fix pagination and globalization bugs. Dec 10, 2019
avanigupta
avanigupta reviewed Dec 10, 2019
View reviewed changes
@avanigupta
Copy link
Copy Markdown
Member

avanigupta commented Dec 10, 2019 

    url = 'https://{}{}'.format(endpoint, query_url)

Do we not need to append api version here? Same for unlock and delete urls.

Refers to: src/azure-cli/azure/cli/command_modules/appconfig/_azconfig/azconfig_client.py:319 in f75c1d3. [](commit_id = f75c1d3, deletion_comment = False)

@avanigupta
Copy link
Copy Markdown
Member

avanigupta commented Dec 10, 2019 

def delete_keyvalue_by_key_label(self, key, label=None, modify_options=None):

Looks like dead code. Can you remove it please?

Refers to: src/azure-cli/azure/cli/command_modules/appconfig/_azconfig/azconfig_client.py:153 in f75c1d3. [](commit_id = f75c1d3, deletion_comment = False)

@shenmuxiaosen
Copy link
Copy Markdown
Contributor Author

shenmuxiaosen commented Dec 10, 2019 

def delete_keyvalue_by_key_label(self, key, label=None, modify_options=None):

This is part of our python SDK. We have two methods to delete key values. delete_keyvalue(keyvalue) will check etag and fail if mismatch. For delete_keyvalue_by_key_label(key, label) it is a force delete that won't check pre condition. This may not be used in CLI, but we should keep it in internal python sdk. We may consider to use the official python SDK once it is stabilized.

In reply to: 564198076 [](ancestors = 564198076)

Refers to: src/azure-cli/azure/cli/command_modules/appconfig/_azconfig/azconfig_client.py:153 in f75c1d3. [](commit_id = f75c1d3, deletion_comment = False)

@shenmuxiaosen
Copy link
Copy Markdown
Contributor Author

shenmuxiaosen commented Dec 10, 2019 

    url = 'https://{}{}'.format(endpoint, query_url)

yes, good catch. I will add it. Same with unlock and delete.

In reply to: 564196480 [](ancestors = 564196480)

Refers to: src/azure-cli/azure/cli/command_modules/appconfig/_azconfig/azconfig_client.py:319 in f75c1d3. [](commit_id = f75c1d3, deletion_comment = False)

avanigupta
avanigupta approved these changes Dec 11, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@avanigupta avanigupta 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!

@Juliehzl Juliehzl self-requested a review December 12, 2019 01:49
@Juliehzl Juliehzl self-assigned this Dec 12, 2019
@Juliehzl
Copy link
Copy Markdown
Contributor

Juliehzl commented Dec 12, 2019

Hi @shenmuxiaosen, please update history.rst for your change.

@mmyyrroonn mmyyrroonn mentioned this pull request Dec 12, 2019
Merged
3 tasks
@Juliehzl
Copy link
Copy Markdown
Contributor

Juliehzl commented Dec 12, 2019

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Dec 12, 2019

Azure Pipelines successfully started running 1 pipeline(s).

@Juliehzl Juliehzl merged commit 78e201f into Azure:dev Dec 12, 2019
@shenmuxiaosen shenmuxiaosen deleted the user/shuai/pagination branch May 4, 2020 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Juliehzl Juliehzl Juliehzl approved these changes

+1 more reviewer

@avanigupta avanigupta avanigupta approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@shenmuxiaosen shenmuxiaosen

@Juliehzl Juliehzl

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@shenmuxiaosen @avanigupta @Juliehzl