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.

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

[Resources] Add Deployment scripts as a new resource type - preview - #7486

Merged
merged 5 commits into from
Oct 22, 2019
Merged

[Resources] Add Deployment scripts as a new resource type - preview - #7486

merged 5 commits into from
Oct 22, 2019

Conversation

filizt
Copy link
Contributor

@filizt filizt commented Oct 14, 2019

Here's the link to the approved swagger: Azure/azure-rest-api-specs-pr#924

Closing the PR which was created against DeploymentScripts branch.
#7485

Latest improvements:

MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.

Contribution checklist:

  • I have reviewed the documentation for the workflow.
  • Validation tools were run on swagger spec(s) and have all been fixed in this PR.
  • The OpenAPI Hub was used for checking validation status and next steps.

ARM API Review Checklist

  • Service team MUST add the "WaitForARMFeedback" label if the management plane API changes fall into one of the below categories.
  • adding/removing APIs.
  • adding/removing properties.
  • adding/removing API-version.
  • adding a new service in Azure.

Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs.

  • If you are blocked on ARM review and want to get the PR merged urgently, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
    Please follow the link to find more details on API review process.

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Oct 14, 2019

In Testing, Please Ignore

[Logs] (Generated from 311c8c8, Iteration 8)

Warning .NET: test-repo-billy/azure-sdk-for-net [Logs] [Diff]
  • No packages generated.
Failed Python: test-repo-billy/azure-sdk-for-python [Logs]
  • No packages generated.
Failed Java: test-repo-billy/azure-sdk-for-java [Logs] [Diff]
  • Failed features/resource-manager/v2015_12_01 [Logs]
  • Failed locks/resource-manager/v2016_09_01 [Logs]
  • Failed policy/resource-manager/v2016_12_01 [Logs]
  • Failed policy/resource-manager/v2018_03_01 [Logs]
  • Failed policy/resource-manager/v2018_05_01 [Logs]
  • Failed policy/resource-manager/v2019_01_01 [Logs]
  • Failed policy/resource-manager/v2019_06_01 [Logs]
  • Failed resources/resource-manager/v2016_06_01 [Logs]
  • Failed resources/resource-manager/v2016_09_01 [Logs]
  • Failed resources/resource-manager/v2018_02_01 [Logs]
  • Failed resources/resource-manager/v2018_06_01 [Logs]
  • Failed resources/resource-manager/v2019_03_01 [Logs]
  • Failed resources/resource-manager/v2019_05_01 [Logs]
  • Failed resources/resource-manager/v2019_05_10 [Logs]
  • Failed resources/resource-manager/v2019_06_01 [Logs]
  • Failed resources/resource-manager/v2019_07_01 [Logs]
  • Failed resources/resource-manager/v2019_08_01 [Logs]
Failed Go: test-repo-billy/azure-sdk-for-go [Logs] [Diff]
Failed JavaScript: test-repo-billy/azure-sdk-for-js [Logs] [Diff]

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@AutorestCI
Copy link

AutorestCI commented Oct 14, 2019

Automation for azure-sdk-for-python

Nothing to generate for azure-sdk-for-python

@AutorestCI
Copy link

AutorestCI commented Oct 14, 2019

Automation for azure-sdk-for-go

Nothing to generate for azure-sdk-for-go

},
"arguments": {
"type": "string",
"description": "Command line arguments to pass to the script. Arguments are seperated by spaces. ex: -Name blue* -Location 'West US 2' "
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"description": "Command line arguments to pass to the script. Arguments are seperated by spaces. ex: -Name blue* -Location 'West US 2' "
"description": "Command line arguments to pass to the script. Arguments are separated by spaces. ex: -Name blue* -Location 'West US 2' "

@Juliehzl
Copy link
Contributor

It is already approved in private repo. LGTM. @filizt can you try to make CI pass?

@filizt
Copy link
Contributor Author

filizt commented Oct 17, 2019

@Juliehzl the swagger is already approved by ARM. See the private repo PR here: https://github.com/Azure/azure-rest-api-specs-pr/pull/924. Could you merge please?

update package name

Co-Authored-By: Yeming Liu <felix_liu@outlook.com>
- from: deploymentScripts.json
suppress: TrackedResourceGetOperation
where: $.definitions.AzureCliScript
reason: Tooling issue.
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious what the actual tool issue is

Copy link
Contributor Author

@filizt filizt Oct 17, 2019

Choose a reason for hiding this comment

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

@nschonni we asked about the linter errors to the swagger team and they directed us to suppress the errors. Below is the copy of the response in the email thread and the errors we saw:


After syncing up with Zhiqing offline. The errors you mentioned are actually false positive. You can add suppression section in readme.md. Such as

directive:
  - suppress: TrackedResourceGetOperation
    reason: <reason here>
    where:
                - $.definitions.CliScript
  - suppress: TrackedResourcePatchOperation
reason: <reason here>
where:
                - $.definitions.PowerShellScript

After debug it. The reason is complex. The rule “TrackedResourceGetOperation” and “TrackedResourcePatchOperation” not only check whether existing “get” or “patch” operation but also check “response.schema.reference” whether equal with name of definition. In your case, The name of CLIScript and PowerShellScript is different from the “response.schema.reference”. So the lint tool report the errors.

Thanks,
Ruoxuan

Errors

ERROR (TrackedResourceGetOperation/R3025/ARMViolation): Tracked resource 'PowerShellScript' must have a get operation.
- file:///C:/Workspace/Git/azure-rest-api-specs-pr/specification/DeploymentScripts/resource-manager/Microsoft.Blueprint/preview/2019-10-01-preview/deploymentScripts.json:397:4 ($.definitions.PowerShellScript)

ERROR (TrackedResourceGetOperation/R3025/ARMViolation): Tracked resource 'CliScript' must have a get operation.
- file:///C:/Workspace/Git/azure-rest-api-specs-pr/specification/DeploymentScripts/resource-manager/Microsoft.Blueprint/preview/2019-10-01-preview/deploymentScripts.json:438:4 ($.definitions.CliScript)

WARNING (TrackedResourceListByResourceGroup/R3027/ARMViolation): The tracked resource, 'DeploymentScript', must have a list by resource group operation.(This rule does not apply for tenant level resources.)
- file:///C:/Workspace/Git/azure-rest-api-specs-pr/specification/DeploymentScripts/resource-manager/Microsoft.Blueprint/preview/2019-10-01-preview/deploymentScripts.json:312:4 ($.definitions.DeploymentScript)

WARNING (TrackedResourceListByResourceGroup/R3027/ARMViolation): The tracked resource, 'PowerShellScript', must have a list by resource group operation.(This rule does not apply for tenant level resources.)
- file:///C:/Workspace/Git/azure-rest-api-specs-pr/specification/DeploymentScripts/resource-manager/Microsoft.Blueprint/preview/2019-10-01-preview/deploymentScripts.json:397:4 ($.definitions.PowerShellScript)

WARNING (TrackedResourceListByResourceGroup/R3027/ARMViolation): The tracked resource, 'CliScript', must have a list by resource group operation.(This rule does not apply for tenant level resources.)
- file:///C:/Workspace/Git/azure-rest-api-specs-pr/specification/DeploymentScripts/resource-manager/Microsoft.Blueprint/preview/2019-10-01-preview/deploymentScripts.json:438:4 ($.definitions.CliScript)

WARNING (TrackedResourceListBySubscription/R3028/ARMViolation): The tracked resource, 'PowerShellScript', must have a list by subscriptions operation.
- file:///C:/Workspace/Git/azure-rest-api-specs-pr/specification/DeploymentScripts/resource-manager/Microsoft.Blueprint/preview/2019-10-01-preview/deploymentScripts.json:397:4 ($.definitions.PowerShellScript)

WARNING (TrackedResourceListBySubscription/R3028/ARMViolation): The tracked resource, 'CliScript', must have a list by subscriptions operation.
- file:///C:/Workspace/Git/azure-rest-api-specs-pr/specification/DeploymentScripts/resource-manager/Microsoft.Blueprint/preview/2019-10-01-preview/deploymentScripts.json:438:4 ($.definitions.CliScript)

ERROR (TrackedResourcePatchOperation/R3026/ARMViolation): Tracked resource 'PowerShellScript' must have patch operation that at least supports the update of tags. It's strongly recommended that the PATCH operation supports update of all mutable properties as well.
- file:///C:/Workspace/Git/azure-rest-api-specs-pr/specification/DeploymentScripts/resource-manager/Microsoft.Blueprint/preview/2019-10-01-preview/deploymentScripts.json:397:4 ($.definitions.PowerShellScript)

ERROR (TrackedResourcePatchOperation/R3026/ARMViolation): Tracked resource 'CliScript' must have patch operation that at least supports the update of tags. It's strongly recommended that the PATCH operation supports update of all mutable properties as well.
- file:///C:/Workspace/Git/azure-rest-api-specs-pr/specification/DeploymentScripts/resource-manager/Microsoft.Blueprint/preview/2019-10-01-preview/deploymentScripts.json:438:4 ($.definitions.CliScript)
PS C:\Workspace\Git\azure-rest-api-specs-pr\specification\DeploymentScripts\resource-manager>

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! I just like kicking the tires on the tools, and like seeing where the limitations pop up

Copy link
Contributor

@KrisBash KrisBash left a comment

Choose a reason for hiding this comment

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

PR was previously reviewed in private repo

@KrisBash KrisBash 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 Oct 18, 2019
@yungezz
Copy link
Member

yungezz commented Oct 21, 2019

@Juliehzl could you pls merge the PR? thanks.

tag: package-deploymentscripts-2019-10-preview
```

### Tag: package-resources-2019-10-preview
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be package-deploymentscripts-2019-10-preview?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tag name change is fine with us. It sounds like it'll be more clear this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants