Skip to content

Conversation

@yingru97
Copy link
Contributor

@yingru97 yingru97 commented Jan 13, 2022

Description

Migrate APIM CLI to track2

Testing Guide

Use ApimScenarioTest to test it

History Notes


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

@yonzhan yonzhan requested a review from jsntcy January 14, 2022 00:01
@yonzhan yonzhan requested a review from kairu-ms January 14, 2022 00:01
@yonzhan yonzhan added this to the Jan 2022 (2022-02-08) milestone Jan 14, 2022
@yonzhan
Copy link
Collaborator

yonzhan commented Jan 14, 2022

track2 migration

@yingru97 yingru97 changed the title migrate CLI to track2 {APIM} migrate CLI to track2 Jan 14, 2022
@jsntcy
Copy link
Member

jsntcy commented Feb 14, 2022

/azp run

@azure-pipelines
Copy link

Pull request contains merge conflicts.

@jsntcy
Copy link
Member

jsntcy commented Feb 14, 2022

@yingru97, please resolve the conflict first

@yingru97 yingru97 force-pushed the yingru97/apim-cli-track2 branch from 1f45a9d to cddca6e Compare February 14, 2022 22:26
query=subscription_key_query_param_name
)
elif subscription_key_query_param_name is not None or subscription_key_header_name is not None:
raise CLIError(
Copy link
Member

@jsntcy jsntcy Feb 15, 2022

Choose a reason for hiding this comment

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

Please use a more specific error type from azure.cli.core.azclierror import RequiredArgumentMissingError instead of the generic error CLIError.
BTW, please check other errors you used to see if they also need update error type. #Closed

@jsntcy
Copy link
Member

jsntcy commented Feb 15, 2022

@yingru97, Overall LGTM, just one minor suggestion, please check.

parameters.value = specification_url
elif specification_path is not None and specification_url is not None:
raise CLIError(
raise ArgumentUsageError(
Copy link
Member

@jsntcy jsntcy Feb 16, 2022

Choose a reason for hiding this comment

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

Use MutuallyExclusiveArgumentError instead of ArgumentUsageError as (ArgumentUsageError is fallback of the argument usage related errors. Avoid using this class unless the error can not be classified into the Argument related specific error types.) #Closed

"Can't specify specification-url and specification-path at the same time.")
else:
raise CLIError(
raise ArgumentUsageError(
Copy link
Member

@jsntcy jsntcy Feb 16, 2022

Choose a reason for hiding this comment

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

Use RequiredArgumentMissingError #Closed

query=subscription_key_query_param_name
)
elif subscription_key_query_param_name is not None or subscription_key_header_name is not None:
raise ArgumentUsageError(
Copy link
Member

@jsntcy jsntcy Feb 16, 2022

Choose a reason for hiding this comment

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

Use RequiredArgumentMissingError #Closed

subscription_key_header_name, subscription_key_query_param_name)
resource.api_version = api_version
resource.api_version_set_id = _get_vs_fullpath(api_version_set_id)
raise InvalidArgumentValueError(
Copy link
Member

@jsntcy jsntcy Feb 18, 2022

Choose a reason for hiding this comment

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

Miss importing "InvalidArgumentValueError" which makes CI failed, please fix it.

@jsntcy jsntcy merged commit 7d555ef into Azure:dev Feb 18, 2022
@kairu-ms kairu-ms added the API Management az apim label Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants