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

Introducing assessments and assessmentsMetadata types #5649

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@chlahav
Copy link
Contributor

commented Apr 15, 2019

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.
@AutorestCI

This comment has been minimized.

Copy link

commented Apr 15, 2019

Automation for azure-sdk-for-ruby

Encountered a Subprocess error: (azure-sdk-for-ruby)

Command: ['/usr/local/bin/autorest', '/tmp/tmp_fm5xnow/rest/specification/security/resource-manager/readme.md', '--perform-load=false', '--swagger-to-sdk', '--output-artifact=configuration.json', '--input-file=foo', '--output-folder=/tmp/tmpg3p0nx4l']
Finished with return code 7
and output:

AutoRest code generation utility [version: 2.0.4283; node: v8.12.0]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
Failure:
Error: Unable to start AutoRest Core from /root/.autorest/@microsoft.azure_autorest-core@2.0.4370/node_modules/@microsoft.azure/autorest-core
Error: Unable to start AutoRest Core from /root/.autorest/@microsoft.azure_autorest-core@2.0.4370/node_modules/@microsoft.azure/autorest-core
    at main (/opt/node_modules/autorest/dist/app.js:232:19)
    at <anonymous>

/root/.autorest/@microsoft.azure_autorest-core@2.0.4370/node_modules/@microsoft.azure/autorest-core/dist/app.js:33
    autorest_core_1.Shutdown();
    ^
ReferenceError: autorest_core_1 is not defined
    at process.on (/root/.autorest/@microsoft.azure_autorest-core@2.0.4370/node_modules/@microsoft.azure/autorest-core/dist/app.js:33:5)
    at emitOne (events.js:121:20)
    at process.emit (events.js:211:7)
    at process.emit (/node_modules/source-map-support/source-map-support.js:439:21)
fs.js:612
  return binding.close(fd);
                 ^

Error: EBADF: bad file descriptor, close
    at Object.fs.closeSync (fs.js:612:18)
    at StaticVolumeFile.shutdown (/opt/node_modules/autorest/dist/static-loader.js:352:10)
    at StaticFilesystem.shutdown (/opt/node_modules/autorest/dist/static-loader.js:406:17)
    at process.exit.n [as exit] (/opt/node_modules/autorest/dist/static-loader.js:169:11)
    at printErrorAndExit (/node_modules/source-map-support/source-map-support.js:423:11)
    at process.emit (/node_modules/source-map-support/source-map-support.js:435:16)
    at process._fatalException (bootstrap_node.js:391:26)

@AutorestCI

This comment has been minimized.

Copy link

commented Apr 15, 2019

Automation for azure-sdk-for-python

A PR has been created for you based on this PR content.

Once this PR will be merged, content will be added to your service PR:
Azure/azure-sdk-for-python#4979

@AutorestCI

This comment has been minimized.

Copy link

commented Apr 15, 2019

Automation for azure-sdk-for-js

A PR has been created for you:
Azure/azure-sdk-for-js#2841

@AutorestCI

This comment has been minimized.

Copy link

commented Apr 15, 2019

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

@AutorestCI

This comment has been minimized.

Copy link

commented Apr 15, 2019

Automation for azure-sdk-for-go

Nothing to generate for azure-sdk-for-go

@azuresdkci

This comment has been minimized.

Copy link
Collaborator

commented Apr 15, 2019

Can one of the admins verify this patch?

@jhendrixMSFT

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

@fearthecowboy I'm not sure what this modeler failure means, can you please elaborate? Oh ok I see, it's complaining because ResourceDetails contains no properties.

@chlahav chlahav force-pushed the chlahav:master branch from 978418e to a09ca2f Apr 17, 2019

@jhendrixMSFT
Copy link
Member

left a comment

The following error needs to be fixed.
Swagger document contains two or more x-ms-enum extensions with the same name 'ReportedSeverity' and different values: Informational,Low,Medium,High vs. Compute,Network,Data,IdentityAndAccess,IoT

@chlahav chlahav force-pushed the chlahav:master branch from a09ca2f to 5853141 Apr 17, 2019

@chlahav chlahav requested a review from jhendrixMSFT Apr 17, 2019

adxsdknet added a commit to adxsdknet/azure-sdk-for-net that referenced this pull request Apr 17, 2019

.NET SDK Resource Provider:'SecurityCenter'
REST Spec PR 'Azure/azure-rest-api-specs#5649'
REST Spec PR Author 'chlahav'
REST Spec PR Last commit

@chlahav chlahav force-pushed the chlahav:master branch from 5853141 to b5988e0 Apr 24, 2019

adxsdknet added a commit to adxsdknet/azure-sdk-for-net that referenced this pull request Apr 24, 2019

.NET SDK Resource Provider:'SecurityCenter'
REST Spec PR 'Azure/azure-rest-api-specs#5649'
REST Spec PR Author 'chlahav'
REST Spec PR Last commit

@chlahav chlahav force-pushed the chlahav:master branch from b5988e0 to fa529ec Apr 24, 2019

@chlahav

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

@jhendrixMSFT all you comments were resolved, can you please review again?

@jhendrixMSFT

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

@chlahav for SDK this looks good, still need ARM to sign off. Have these APIs been deployed? We can't merge until they're live.

@chlahav

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

@jhendrixMSFT these APIs are not exposed since we need ARM review to open them in the manifest.
after the ARM review we will expose the endpoint in the manifest, validate the quality and remove the DoNotMerge label so the PR will be completed

@dsgouda

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

startbuild

adxsdknet added a commit to adxsdknet/azure-sdk-for-net that referenced this pull request Apr 30, 2019

.NET SDK Resource Provider:'SecurityCenter'
REST Spec PR 'Azure/azure-rest-api-specs#5649'
REST Spec PR Author 'chlahav'
REST Spec PR Last commit

@Azure Azure deleted a comment from adxsdknet Apr 30, 2019

@chlahav chlahav force-pushed the chlahav:master branch 2 times, most recently from f2ff02f to 2aca7b2 May 5, 2019

adxsdknet added a commit to adxsdknet/azure-sdk-for-net that referenced this pull request May 6, 2019

.NET SDK Resource Provider:'SecurityCenter'
REST Spec PR 'Azure/azure-rest-api-specs#5649'
REST Spec PR Author 'chlahav'
REST Spec PR Last commit
@chlahav

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

The following error needs to be fixed.
Swagger document contains two or more x-ms-enum extensions with the same name 'ReportedSeverity' and different values: Informational,Low,Medium,High vs. Compute,Network,Data,IdentityAndAccess,IoT

@jhendrixMSFT fixed

}
},
"paths": {
"/{scope}/providers/Microsoft.Security/assessments": {

This comment has been minimized.

Copy link
@majastrz

majastrz May 7, 2019

Contributor

scope [](start = 7, length = 5)

When implementing the service, please ensure that unexpected types of scopes don't cause a 500 and produce a good error message. Also, I would suggest documenting supported scopes in your docs. #Closed

This comment has been minimized.

Copy link
@chlahav

chlahav May 12, 2019

Author Contributor

the plan is to use the CloudError format with the NotFound HTTP status for unsupported types #Closed

This comment has been minimized.

Copy link
@majastrz

majastrz May 13, 2019

Contributor

Should be fine. Thanks!


In reply to: 283122927 [](ancestors = 283122927)

"type": "string",
"description": "Human readable description for the reason the assessment result status"
},
"additionalData": {

This comment has been minimized.

Copy link
@majastrz

majastrz May 7, 2019

Contributor

additionalData [](start = 9, length = 14)

Are these known ahead of time or completely arbitrary? #Closed

This comment has been minimized.

Copy link
@chlahav

chlahav May 12, 2019

Author Contributor

completely arbitrary,
the set of additional data keys are decided per assessment (we have more than 50 assessments and growing) and currently the set is still changing for some of the assessments.
one day it might be more stable, but we will have more types than we can easily document #Closed

"description": "Details of the resource that was assessed",
"discriminator": "assessedResourceSource",
"properties": {
"assessedResourceSource": {

This comment has been minimized.

Copy link
@majastrz

majastrz May 7, 2019

Contributor

assessedResourceSource [](start = 9, length = 22)

This should probably be an enum that only allows "Azure" as the value if you're using it as a discriminator. #Closed

This comment has been minimized.

Copy link
@chlahav

chlahav May 12, 2019

Author Contributor

done, also added OnPremise value because one valued enum is not included in the .Net ctors and creates a breaking change when the new value is added.
we will add the relevant OnPremiseResourceDetails before the completion of this PR, but we still need to work on them #Closed

@majastrz
Copy link
Contributor

left a comment

Can you take a look at my comments and questions?

@Azure Azure deleted a comment from adxsdknet May 8, 2019

@dsgouda

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

startbuild

adxsdknet added a commit to adxsdknet/azure-sdk-for-net that referenced this pull request May 8, 2019

.NET SDK Resource Provider:'SecurityCenter'
REST Spec PR 'Azure/azure-rest-api-specs#5649'
REST Spec PR Author 'chlahav'
REST Spec PR Last commit
@adxsdknet

This comment has been minimized.

Copy link

commented May 8, 2019

Automation for azure-sdk-for-net

A PR has been created for you:
Azure/azure-sdk-for-net#6207
.NET SDK Commits:
adxsdknet/azure-sdk-for-net@3a36eb3
adxsdknet/azure-sdk-for-net@0914506

@chlahav

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

@majastrz the requested changes were pushed, can you please review?

adxsdknet added a commit to adxsdknet/azure-sdk-for-net that referenced this pull request May 12, 2019

.NET SDK Resource Provider:'SecurityCenter'
REST Spec PR 'Azure/azure-rest-api-specs#5649'
REST Spec PR Author 'chlahav'
REST Spec PR Last commit
@majastrz
Copy link
Contributor

left a comment

Looks good.

@jhendrixMSFT

This comment has been minimized.

Copy link
Member

commented May 15, 2019

Now that #5929 has been merged into master can you please merge master into your fork to pick up those changes?

@jhendrixMSFT

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

@chlahav hello! What's the status of this PR?

@chlahav

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

@chlahav hello! What's the status of this PR?

we are still working on validating the API,
it is still not ready to public preview yet so we don't want to merge it

we will update this PR right beforehand to save you guys unnecessary reviews

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.