Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add deleteAll parameter #2078

Merged
merged 1 commit into from
Dec 14, 2017
Merged

Add deleteAll parameter #2078

merged 1 commit into from
Dec 14, 2017

Conversation

shutchings
Copy link
Contributor

@shutchings shutchings commented Dec 1, 2017 •
edited by azuresdkci
Loading

If deleteAll=true then all resources that were created by MLC will get deleted. Currently only the operationalization cluster and ACS cluster get deleted which leaves behind all the ACS VMs.

PR information

  • The title of the PR is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
  • Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
  • If applicable, the PR references the bug/issue that it fixes.
  • Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

@shutchings
Copy link
Contributor Author

@ashvins

shutchings
shutchings commented Dec 4, 2017
View reviewed changes
...urce-manager/Microsoft.MachineLearningCompute/2017-08-01-preview/machineLearningCompute.json
@@ -882,11 +892,11 @@
},
"cert": {
"type": "string",
"description": "The SSL cert data in PEM format encoded as base64 string"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No functional changes here. The cert/key were never being passed as base64 string so I'm fixing the comment.

@azuresdkciprbot
Copy link

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/machinelearningcompute/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 0
After the PR: Warning(s): 0 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@anuchandy anuchandy added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Dec 5, 2017
@anuchandy
Copy link
Member

@simongdavies this PR adds an optional query parameter. Adding WaitForARMFeedback as introducing new property, parameter requires ARM to signoff.

@anuchandy
Copy link
Member

@simongdavies can you please take a look?

ravbhatnagar
ravbhatnagar reviewed Dec 7, 2017
View reviewed changes
...urce-manager/Microsoft.MachineLearningCompute/2017-08-01-preview/machineLearningCompute.json
@@ -486,6 +489,13 @@
"type": "string",
"description": "The version of the Microsoft.MachineLearningCompute resource provider API to use.",
"required": true
},
Copy link
Contributor

Choose a reason for hiding this comment

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

@shutchings I don't entirely follow the usecase/intent of this parameter. Can you please share some more details with an example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When creating an operationalization cluster the resource is placed in the user's resource group and the supporting resources (ACS, app insights, ACR, storage account) are placed in a separate resource group. Right now when the user asks to delete the operationalization cluster only ACS and the operationalization cluster are deleted; ACR, storage accounts, and other resources are not deleted. This is so they still have access to their docker images, logs, telemetry, etc. But some users don't want any resources left behind when doing the delete since they no longer need any of it. With deleteAll=true all resources associated with the operationalization cluster are deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ravbhatnagar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ravbhatnagar @anuchandy can I get some feedback on this? I'd like to start working on the .NET SDK/PowerShell changes.

@azuresdkciprbot
Copy link

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/machinelearningcompute/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 0
After the PR: Warning(s): 0 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@ravbhatnagar
Copy link
Contributor

Looks good.

@ravbhatnagar ravbhatnagar added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Dec 14, 2017
@anuchandy anuchandy merged commit 33045ec into Azure:current Dec 14, 2017
@AutorestCI
Copy link

No modification for AutorestCI/azure-sdk-for-ruby

@AutorestCI AutorestCI mentioned this pull request Dec 14, 2017
Merged
@AutorestCI
Copy link

Build Status Azure/azure-sdk-for-python#1712

@shutchings shutchings mentioned this pull request Dec 18, 2017
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ravbhatnagar ravbhatnagar ravbhatnagar left review comments

@ashvins ashvins ashvins approved these changes

@anuchandy anuchandy anuchandy approved these changes

@simongdavies simongdavies Awaiting requested review from simongdavies

Assignees

@anuchandy anuchandy

Labels
ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review potential-sdk-breaking-change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@shutchings @azuresdkciprbot @anuchandy @ravbhatnagar @AutorestCI @ashvins