Skip to content

Conversation

@kalyanaj
Copy link
Contributor

@kalyanaj kalyanaj commented Oct 3, 2018

Added support for new preview API version (2018-09-15-preview) functionality: Support for domain operations, advanced filters, event subscription expiration. Also, consolidated/simplified existing event subscription CRUD operation parameters to improve usability.

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

General Guidelines

  • Have you run ./scripts/ci/test_static.sh locally? (pip install pylint flake8 required)
  • Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

@williexu
Copy link
Contributor

Please pass CI checks

@kalyanaj
Copy link
Contributor Author

@williexu , @tjprescott : I have resolved all the CI check issues, can you please help review this today? Also, since this depends on the Knack fix (0.4.4), should I specify the minimum version of Knack somewhere?

Copy link
Member

@tjprescott tjprescott left a comment

Choose a reason for hiding this comment

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

There's a lot of copy-paste duplication in the params.py file. You should strive to avoid this since it creates a maintenance burden moving forward. You could either:
a) extract the common info to CliArgumentTypes which you can then register at each scope

foo_type = CliArgumentType(…)
with self.argument_context('scope a') as c:
  c.argument('foo', foo_type)

with self.argument_context('scope b') as c:
  c.argument('foo', foo_type)

b) use scope to only need to register the argument once:

for scope in ['scope a', 'scope b']:
  with self.argument_context(scope) as c:
    c.argument('foo', ...)


with self.argument_context('eventgrid event-subscription create') as c:
c.argument('source_resource_id', help="Fully qualified identifier of the Azure resource to which the event subscription needs to be created.")
c.argument('resource_id', deprecate_info=c.deprecate(redirect="--source-resource-id", expiration='2.1.0', hide=True))
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to extract this common info to a CliCommandType so that if you have to change it later, it is changed everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback, I have updated this now to remove this duplication.

@williexu
Copy link
Contributor

williexu commented Oct 15, 2018

@tjprescott is the CLI ready to take a dependency on knack v0.4.4?
If so, this extension can specify the new version of the CLI it is valid for once that is released.
Made a PR: Azure/azure-cli#7567

@williexu
Copy link
Contributor

@kalyanaj you will need to update your azext_metadata.json to specify the minimum CLI version as 2.0.48 to rely on the new knack version.

@kalyanaj
Copy link
Contributor Author

Thanks for the info and for PR 7567, @williexu. I will update it as you suggested once CLI 2.0.48 gets released, until then leaving it as it is so as to not blocking testing/initial release.

@kalyanaj
Copy link
Contributor Author

@tjprescott , @williexu : I have made all the changes discussed above. Can you please review/merge if there are no additional feedback items?

@williexu
Copy link
Contributor

@kalyanaj can you actually set the min cli version to 2.0.49?
We just released a hot fix version (2.0.48)

@williexu
Copy link
Contributor

I can merge right after

@williexu williexu merged commit be5e684 into Azure:master Oct 17, 2018
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.

3 participants