Skip to content

Conversation

@frankqianms
Copy link
Member

@frankqianms frankqianms commented Jan 26, 2025


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

Related command

az apic api-analysis
az apic integration
az apic import-from-apim

  1. Newly added az apic api-analysis commands (in preview).
  2. Removed the preview tag of az apic integration
  3. Depracated az apic import-from-apim
image image

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally? (pip install wheel==0.30.0 required)
  • My extension version conforms to the Extension version schema

For new extensions:

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update src/index.json automatically.
You only need to update the version information in file setup.py and historical information in file HISTORY.rst in your PR but do not modify src/index.json.

blackchoey and others added 30 commits April 9, 2024 09:51
* build: Run CI in Python 3.8-3.11

* build: remove pull request event for CI to avoid duplicate runs
* feat: enable openapi spec from url in api register

* refactor: set spec definition as link format when link provided

* fix: fix style

* test: add error handling case for testing invalid spec url

* fix: fix test case

* fix: use 404 response url

* test: update case

* test: update test case

* refactor: update error logic
* test: update test case to setup live test pipeline (#75)

* test: update test case

* update

* .

* .

* .

* .

* .

* .

* .

* .

* .

* .

* .

* test: update test case

* refactor: enable both identity

* fix: bad if else

* fix: fix bad parameter
* refactor: add example

* fix: update params

* fix: bad api id
* refactor: add example

* fix: update params

* fix: bad api id

* refactor: add @filename.json examples

* refactor: update
* refactor: add error handling

* refactor: catch internal error

* fix: revert the change
* feat: add APIM/APIC sync commands

* feat: rename apim to azure-api-management

* style: fix code style

* fix: sync property names with new API spec

* Revert "fix: sync property names with new API spec"

This reverts commit 04da67e.

---------

Co-authored-by: frankqianms <[email protected]>
* feat: resolve feedback and fix examples

* style: fix code style
* feat: add APIM/APIC sync commands

* feat: rename apim to azure-api-management

* style: fix code style

* fix: sync property names with new API spec

* feat: add aws api gateway sync command

* Revert "fix: sync property names with new API spec"

This reverts commit 04da67e.

* refactor: add amazonApiGatewaySource

* refactor: refactoring apim sync and amazon sync

* refactor: refactor cmd structure to make apim and aws sync seperated

* fix: remove log print

* chore: generate new cmds

* refactor: update version and remove import

* feat: add `apic integration create amazon-api-gateway`

* fix: style

* fix: change query param api-version

* revert changes in _delete.py

* fix: some neede fixs

* fix: add the help sentence

* refactor: make params clear

* refactor: handle msi-resource-id

* refacor: revert flatten of apim resource

* fix: use 06-01-preiew currently

* fix: style

* refactor: arg groups

* fix: bad short param name

* chore: re-generate

* fix: old resource_id name

* chore: arg group

* chore: naming

* fix: fix according to comments

* chore: update

* fix: style

---------

Co-authored-by: Chaoyi Yuan <[email protected]>
* feat: add import amazon-api-gateway cmd

* feat: change arg group and update parameter name

---------

Co-authored-by: Chaoyi Yuan <[email protected]>
* feat: rename command and param names

* doc: update comments

* doc: update sample
…apic integration create aws` (#86)

* test: add test case for apim sync

* refactor: refactor for apim preparer

* refactor: refactoring case and utils, optimize checkers

* chore: remove print and add explaination

* refactor: rename file

* fix: try to fix error determing the version

* revert: Remove specific azure-cli and azure-core installations

* test: add aws sync testcase (#87)

* test: add test case for aws sync command

* fix: remove key value

* fix: remove pip install

* chore: renaming constants

* refactor: update the utils and test case

* refactor: updated
* refactor: integration examples

* fix: apic update example
* feat: analysi rule init

* feat: add create cmd

* feat: add create and delete api-analysis commands

* feat: add import-ruleset and export-ruleset commands

* fix: update aaz

* fix: registered

* fix: examples

* fix: fix style

* refactor: renaming

* refactor: regenerate aaz

* fix: fix codes

* fix: fix logics

* fix: style

* fix: rename parameter service name

* fix: change api-analysis status to preview

* fix: integration list

* refactor: modify examples

* feat: analysi rule init

* feat: add create cmd

* feat: add create and delete api-analysis commands

* feat: add import-ruleset and export-ruleset commands

* fix: update aaz

* fix: registered

* fix: examples

* fix: fix style

* refactor: renaming

* refactor: regenerate aaz

* fix: fix codes

* fix: fix logics

* fix: style

* fix: change api-analysis status to preview

* fix: change short name of service name

* fix: apic update example

* fix: examples and default value

* chore: example

* fix: bad parameter short names

* fix: downgrade api version

* fix: set default workspace for list,show,update api-analysis

* refactor: integration examples

* fix: style
@frankqianms frankqianms changed the title build: apic-extension v1.2.0b2 release - analysis follow up build: apic-extension v1.2.0b3 release - analysis follow up Aug 4, 2025
@frankqianms frankqianms marked this pull request as ready for review August 6, 2025 03:20
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request releases version 1.2.0b3 of the apic-extension with follow-up analysis work. The changes include a version bump, API test updates for better isolation, and new API analysis functionality.

  • Version update from 1.2.0b2 to 1.2.0b3 in setup.py
  • Added linter exclusions configuration for various update commands
  • Enhanced test infrastructure with new API analysis test framework and updated test data

Reviewed Changes

Copilot reviewed 138 out of 142 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/apic-extension/setup.py Version bump to 1.2.0b3
src/apic-extension/linter_exclusions.yml Added linter rule exclusions for update commands
src/apic-extension/azext_apic_extension/tests/latest/utils.py Enhanced test utilities with API analysis preparer and updated API names
src/apic-extension/azext_apic_extension/tests/latest/test_service_commands.py Updated test cases to use new API naming convention
src/apic-extension/azext_apic_extension/tests/latest/test_register_command.py Fixed OpenAPI specification version format
src/apic-extension/azext_apic_extension/tests/latest/test_integration_commands.py Removed AWS import test case
src/apic-extension/azext_apic_extension/tests/latest/test_assets/ruleset_test/ruleset.yml Added test ruleset configuration
src/apic-extension/azext_apic_extension/tests/latest/test_assets/ruleset_test/functions/greeting.js Added test JavaScript function
src/apic-extension/azext_apic_extension/tests/latest/test_assets/filter.json Added API analysis filter configuration
src/apic-extension/azext_apic_extension/tests/latest/test_apianalysis_commands.py Added comprehensive API analysis test suite
src/apic-extension/azext_apic_extension/tests/latest/recordings/*.yaml Updated test recordings with new API versions and responses
Comments suppressed due to low confidence (1)

src/apic-extension/azext_apic_extension/tests/latest/test_apianalysis_commands.py:22

  • [nitpick] The method name '_delete_existing_analyzer_configs' is inconsistent with the class naming convention. Consider renaming to '_delete_existing_analysis_configs' to match the 'ApiAnalysis' class name.
    async def _delete_existing_analyzer_configs(self):

Comment on lines 34 to 42
await self.cmd('az apic api-analysis delete -g {rg} -n {s} -c {config_name} --yes')
except Exception:
# Ignore errors if deletion fails (e.g., already deleted)
pass

@ResourceGroupPreparer(name_prefix="clirg", location=TEST_REGION, random_name_length=32)
@ApicServicePreparer()
async def test_api_analysis_create(self):
await self._delete_existing_analyzer_configs()
Copy link

Copilot AI Aug 6, 2025

Choose a reason for hiding this comment

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

The 'await' keyword is used with 'self.cmd()' but this method is not asynchronous. The 'cmd' method from ScenarioTest is synchronous and does not return an awaitable.

Suggested change
await self.cmd('az apic api-analysis delete -g {rg} -n {s} -c {config_name} --yes')
except Exception:
# Ignore errors if deletion fails (e.g., already deleted)
pass
@ResourceGroupPreparer(name_prefix="clirg", location=TEST_REGION, random_name_length=32)
@ApicServicePreparer()
async def test_api_analysis_create(self):
await self._delete_existing_analyzer_configs()
self.cmd('az apic api-analysis delete -g {rg} -n {s} -c {config_name} --yes')
except Exception:
# Ignore errors if deletion fails (e.g., already deleted)
pass
@ResourceGroupPreparer(name_prefix="clirg", location=TEST_REGION, random_name_length=32)
@ApicServicePreparer()
def test_api_analysis_create(self):
self._delete_existing_analyzer_configs()

Copilot uses AI. Check for mistakes.
Comment on lines 34 to 42
await self.cmd('az apic api-analysis delete -g {rg} -n {s} -c {config_name} --yes')
except Exception:
# Ignore errors if deletion fails (e.g., already deleted)
pass

@ResourceGroupPreparer(name_prefix="clirg", location=TEST_REGION, random_name_length=32)
@ApicServicePreparer()
async def test_api_analysis_create(self):
await self._delete_existing_analyzer_configs()
Copy link

Copilot AI Aug 6, 2025

Choose a reason for hiding this comment

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

The test method is marked as 'async' but the test framework (ScenarioTest) expects synchronous test methods. This will cause the test to not run properly.

Suggested change
await self.cmd('az apic api-analysis delete -g {rg} -n {s} -c {config_name} --yes')
except Exception:
# Ignore errors if deletion fails (e.g., already deleted)
pass
@ResourceGroupPreparer(name_prefix="clirg", location=TEST_REGION, random_name_length=32)
@ApicServicePreparer()
async def test_api_analysis_create(self):
await self._delete_existing_analyzer_configs()
self.cmd('az apic api-analysis delete -g {rg} -n {s} -c {config_name} --yes')
except Exception:
# Ignore errors if deletion fails (e.g., already deleted)
pass
@ResourceGroupPreparer(name_prefix="clirg", location=TEST_REGION, random_name_length=32)
@ApicServicePreparer()
def test_api_analysis_create(self):
self._delete_existing_analyzer_configs()

Copilot uses AI. Check for mistakes.
try:
self.live_only_execute(self.cli_ctx, cmd)
except HttpResponseError as e:
if e.message.startswith("(ValidationError) Number of analyzer configs for this service"):
Copy link

Copilot AI Aug 6, 2025

Choose a reason for hiding this comment

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

[nitpick] The exception handling checks for a specific error message using 'startswith()' which could be fragile. Consider using more specific exception types or error codes if available from the API.

Suggested change
if e.message.startswith("(ValidationError) Number of analyzer configs for this service"):
error_code = None
# Try to extract error code from the response, if available
if hasattr(e, "response") and e.response is not None:
try:
error_body = e.response.text()
error_json = json.loads(error_body)
# Azure error format: {"error": {"code": "...", "message": "..."}}
error_code = error_json.get("error", {}).get("code")
except Exception:
pass
if (error_code == "ValidationError") or (getattr(e, "status_code", None) == 400):

Copilot uses AI. Check for mistakes.
Comment on lines +489 to +490
pass

Copy link

Copilot AI Aug 6, 2025

Choose a reason for hiding this comment

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

[nitpick] The empty 'pass' statement after setting the name variable makes the logic unclear. Consider adding a comment explaining why no action is needed or restructuring the logic.

Suggested change
pass

Copilot uses AI. Check for mistakes.
self.check('name', 'openapi'), # hard coded when spec is swagger or openapi
self.check('specification.name', 'openapi'),
self.check('specification.version', '3-0-0'),
self.check('specification.version', '3.0.0'),
Copy link
Contributor

Choose a reason for hiding this comment

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

What causes the behavior change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest keep previous behavior if no explicit ask to change it. Not sure whether users rely on existing naming convention to do something.

Copy link
Member Author

@frankqianms frankqianms Aug 6, 2025

Choose a reason for hiding this comment

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

It is from partners ask. And keeping the previous behavior will cause the display format error in the Azure portal.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. Looks good to me as it just changes the display name.

@blackchoey
Copy link
Contributor

LGTM

@kairu-ms
Copy link
Contributor

kairu-ms commented Aug 6, 2025

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@necusjz
Copy link
Member

necusjz commented Aug 7, 2025

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@kairu-ms kairu-ms merged commit cb32570 into Azure:main Aug 7, 2025
24 checks passed
@azclibot
Copy link
Collaborator

azclibot commented Aug 7, 2025

[Release] Update index.json for extension [ apic-extension-1.2.0b3 ] : https://dev.azure.com/msazure/One/_build/results?buildId=132991213&view=results

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants