Skip to content

Conversation

@adewaleo
Copy link
Contributor

@adewaleo adewaleo commented Apr 19, 2019


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.

@adewaleo adewaleo requested a review from yugangw-msft April 19, 2019 00:35
@adewaleo adewaleo force-pushed the proximity-placement-groups branch 2 times, most recently from 93ea962 to 57993c2 Compare April 24, 2019 17:12
@adewaleo adewaleo requested a review from tjprescott April 24, 2019 17:37
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a correct trace message when the PPD is not available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just for debugging purpose to show what was processed for the parameter. I can reword the logging message.

Copy link
Contributor

Choose a reason for hiding this comment

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

what will happen at service end, if we don't do the check at client end? Do we get a good error?

Copy link
Contributor Author

@adewaleo adewaleo Apr 24, 2019

Choose a reason for hiding this comment

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

I believe the error should be something like the following, if the ppg is in a different region or does not exist:

Deployment failed. Correlation ID: 260fc599-b6f4-418f-9c2e-fc800602d78f. {
  "error": {
    "code": "NotFound",
    "message": "Proximity Placement Group '/subscriptions/00977cdb-163f-435f-9c32-39ec8ae61f4d/resourceGroups/ova-test/providers/Microsoft.Compute/proximityPlacementGroups/tosinppg' cannot be found."
  }
}

The CLI does perform this check for availability set... in _validate_vm_create_availability_set. I can remove both existence checks or keep both.-

Copy link
Contributor

Choose a reason for hiding this comment

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

you might need to update other command modules such as acs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes @yugangw-msft acs and servicefabric have been updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be proximity placement group not availability set

Copy link
Member

Choose a reason for hiding this comment

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

Why is this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

debug statement just in case the user enters an invalid proximity placement group. The previous lines try to parse the resource if and return None. Maybe I should actually raise a CLI error if this fails to parse...

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe DiskStorageAccountTypes = DiskStorageAccountTypes or StorageAccountTypes

@adewaleo adewaleo force-pushed the proximity-placement-groups branch 2 times, most recently from f80194b to c983709 Compare April 29, 2019 23:15
@adewaleo adewaleo force-pushed the proximity-placement-groups branch from c983709 to 3a39379 Compare April 29, 2019 23:49
@adewaleo adewaleo merged commit d5835c5 into Azure:dev Apr 30, 2019
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