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

Add APIs for integration runtime sharing feature. #3124

Closed
wants to merge 12 commits into from
Closed

Add APIs for integration runtime sharing feature. #3124

wants to merge 12 commits into from

Conversation

zhangyd2015
Copy link
Contributor

@zhangyd2015 zhangyd2015 commented May 23, 2018

Add APIs for integration runtime sharing feature.
Add two fields 'pushedVersion' 'latestVersion' into SelfHostedIntegrationRuntimeStatus
Fixed issue #2057

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

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

Add two fields 'pushedVersion' 'latestVersion' into SelfHostedIntegrationRuntimeStatus
Fixed issue #2057
@@ -564,7 +651,7 @@
"200": {
"description": "OK.",
"schema": {
"$ref": "#/definitions/IntegrationRuntimeStatusResponse"
"$ref": "#/definitions/IntegrationRuntimeResource"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is to fix issue #2057

Copy link
Contributor

Choose a reason for hiding this comment

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

This fix should go into new API version since it is a breaking change. I'm adding it in my PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Need to remove from here.

@AutorestCI
Copy link

AutorestCI commented May 23, 2018

Automation for azure-sdk-for-python

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

Command: ['/usr/local/bin/autorest', '/tmp/tmpfos_9x_c/rest/specification/datafactory/resource-manager/readme.md', '--multiapi', '--python', '--python-mode=update', '--python-sdks-folder=/tmp/tmpfos_9x_c/sdk', '--use=@microsoft.azure/autorest.python@~3.0', '--version=preview']
Finished with return code 1
and output:

AutoRest code generation utility [version: 2.0.4280; node: v7.10.1]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
   Loading AutoRest core      '/root/.autorest/@microsoft.azure_autorest-core@2.0.4280/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4280)
   Loading AutoRest extension '@microsoft.azure/autorest.python' (~3.0->3.0.52)
   Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.3.44->2.3.44)
FailFast:

   at System.Diagnostics.Debug.Assert(Boolean condition, String message, String detailMessage)
   at AutoRest.Extensions.SwaggerExtensions.FlattenProperty(Property propertyToFlatten, HashSet`1 typesToDelete) in /opt/vsts/work/1/s/autorest.common/src/SwaggerExtensions.cs:line 220
   at AutoRest.Extensions.SwaggerExtensions.FlattenModels(CodeModel codeModel) in /opt/vsts/work/1/s/autorest.common/src/SwaggerExtensions.cs:line 95
   at AutoRest.Python.Azure.TransformerPya.TransformCodeModel(CodeModel cm) in /opt/vsts/work/1/s/src/azure/TransformerPya.cs:line 33
   at AutoRest.Python.Program.<ProcessInternal>d__3.MoveNext() in /opt/vsts/work/1/s/src/Program.cs:line 107
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.Tasks.AwaitTaskContinuation.RunOrScheduleAction(Action action, Boolean allowInlining, Task& currentTask)
   at System.Threading.Tasks.Task.RunContinuations(Object continuationObject)
   at System.Threading.Tasks.Task`1.TrySetResult(TResult result)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.SetExistingTaskResult(TResult result)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.SetResult(TResult result)
   at Microsoft.Perks.JsonRPC.Connection.<Request>d__37`1.MoveNext() in /opt/vsts/work/1/s/autorest.common/src/JsonRpc/Connection.cs:line 371
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.Tasks.AwaitTaskContinuation.RunOrScheduleAction(Action action, Boolean allowInlining, Task& currentTask)
   at System.Threading.Tasks.Task.RunContinuations(Object continuationObject)
   at System.Threading.Tasks.Task`1.TrySetResult(TResult result)
   at System.Threading.Tasks.TaskCompletionSource`1.TrySetResult(TResult result)
   at Microsoft.Perks.JsonRPC.CallerResponse`1.SetCompleted(JToken result) in /opt/vsts/work/1/s/autorest.common/src/JsonRpc/CallerResponse.cs:line 57
   at Microsoft.Perks.JsonRPC.Connection.<>c__DisplayClass27_0.<<Process>b__0>d.MoveNext() in /opt/vsts/work/1/s/autorest.common/src/JsonRpc/Connection.cs:line 289
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder.Start[TStateMachine](TStateMachine& stateMachine)
   at Microsoft.Perks.JsonRPC.Connection.<>c__DisplayClass27_0.<Process>b__0()
   at System.Threading.Tasks.Task`1.InnerInvoke()
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot)
   at System.Threading.ThreadPoolWorkQueue.Dispatch()

   at System.Environment.FailFast(System.String, System.Exception)
   at AutoRest.Extensions.SwaggerExtensions.FlattenProperty(AutoRest.Core.Model.Property, System.Collections.Generic.HashSet`1<System.String>)
   at AutoRest.Extensions.SwaggerExtensions.FlattenModels(AutoRest.Core.Model.CodeModel)
   at AutoRest.Python.Azure.TransformerPya.TransformCodeModel(AutoRest.Core.Model.CodeModel)
   at AutoRest.Python.Program+<ProcessInternal>d__3.MoveNext()
   at System.Threading.ExecutionContext.Run(System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object)
   at System.Threading.Tasks.AwaitTaskContinuation.RunOrScheduleAction(System.Action, Boolean, System.Threading.Tasks.Task ByRef)
   at System.Threading.Tasks.Task.RunContinuations(System.Object)
   at System.Threading.Tasks.Task`1[[System.Nullable`1[[System.Boolean, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].TrySetResult(System.Nullable`1<Boolean>)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1[[System.Nullable`1[[System.Boolean, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].SetExistingTaskResult(System.Nullable`1<Boolean>)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1[[System.Nullable`1[[System.Boolean, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].SetResult(System.Nullable`1<Boolean>)
   at Microsoft.Perks.JsonRPC.Connection+<Request>d__37`1[[System.Nullable`1[[System.Boolean, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].MoveNext()
   at System.Threading.ExecutionContext.Run(System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object)
   at System.Threading.Tasks.AwaitTaskContinuation.RunOrScheduleAction(System.Action, Boolean, System.Threading.Tasks.Task ByRef)
   at System.Threading.Tasks.Task.RunContinuations(System.Object)
   at System.Threading.Tasks.Task`1[[System.Nullable`1[[System.Boolean, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].TrySetResult(System.Nullable`1<Boolean>)
   at System.Threading.Tasks.TaskCompletionSource`1[[System.Nullable`1[[System.Boolean, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].TrySetResult(System.Nullable`1<Boolean>)
   at Microsoft.Perks.JsonRPC.CallerResponse`1[[System.Nullable`1[[System.Boolean, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].SetCompleted(Newtonsoft.Json.Linq.JToken)
   at Microsoft.Perks.JsonRPC.Connection+<>c__DisplayClass27_0+<<Process>b__0>d.MoveNext()
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder.Start[[System.__Canon, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]](System.__Canon ByRef)
   at Microsoft.Perks.JsonRPC.Connection+<>c__DisplayClass27_0.<Process>b__0()
   at System.Threading.Tasks.Task`1[[System.__Canon, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].InnerInvoke()
   at System.Threading.ExecutionContext.Run(System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object)
   at System.Threading.Tasks.Task.ExecuteWithThreadLocal(System.Threading.Tasks.Task ByRef)
   at System.Threading.ThreadPoolWorkQueue.Dispatch()
FATAL: python/generate - FAILED
FATAL: Error: [Exception] AutoRest extension '@microsoft.azure/autorest.python' terminated.
Process() cancelled due to exception : [Exception] AutoRest extension '@microsoft.azure/autorest.python' terminated.

@AutorestCI
Copy link

AutorestCI commented May 23, 2018

Automation for azure-libraries-for-java

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:
AutorestCI/azure-libraries-for-java#18

"type": "string",
"description": "The resource type.",
"readOnly": true
"definitions": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the differences in "definitions" is format changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you guys eliminate these format changes to make sure important changes are not missed?

@AutorestCI
Copy link

AutorestCI commented May 23, 2018

Automation for azure-sdk-for-node

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-node#2770

@AutorestCI
Copy link

AutorestCI commented May 23, 2018

Automation for azure-sdk-for-go

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-go#2008

@annatisch
Copy link
Member

Thanks @zhangyd2015 - there are some model validation errors - could you take a look?
https://travis-ci.org/Azure/azure-rest-api-specs/jobs/382499293

"tags": [
"factories"
],
"operationId": "Factories_ListSharedIntegrationRuntime",
Copy link
Member

Choose a reason for hiding this comment

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

This operation should use the x-ms-pageable extension

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@annatisch Done.

"tags": [
"factories"
],
"operationId": "Factories_ListSharedFactory",
Copy link
Member

Choose a reason for hiding this comment

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

This operation should use the x-ms-pageable extension

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@annatisch Done.

@AutorestCI
Copy link

AutorestCI commented May 24, 2018

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

@zhangyd2015
Copy link
Contributor Author

@annatisch Fixed the model validation errors introduced by my PR. For rest of the validation errors for integration runtime, I guess it might be to oav tool error which can't handle the model well.

@annatisch
Copy link
Member

annatisch commented May 24, 2018

@zhangyd2015 - this error looks like it should be fixable (factoryId should be factoryResourceId):

{ code: 'REQUEST_VALIDATION_ERROR',
  id: 'OAV109',
  message: 'Found errors in validating the request for x-ms-example "Factories_ConfigureFactoryRepo" in operation "Factories_ConfigureFactoryRepo".',
  innerErrors: 
   [ { code: 'INVALID_REQUEST_PARAMETER',
       errors: 
        [ { code: 'OBJECT_ADDITIONAL_PROPERTIES',
            params: [ [ 'factoryId' ] ],
            message: 'Additional properties not allowed: factoryId',
            path: [],
            description: 'Factory\'s VSTS repo information.' } ],
       in: 'body',
       message: 'Invalid parameter (factoryRepoUpdate): Value failed JSON Schema validation',
       name: 'factoryRepoUpdate',
       path: 
        [ 'paths',
          '/subscriptions/{subscriptionId}/providers/Microsoft.DataFactory/locations/{locationId}/configureFactoryRepo',
          'post',
          'parameters',
          '3' ] } ] }

I will take a look at the others.

@annatisch annatisch added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label May 24, 2018
@zhangyd2015
Copy link
Contributor Author

@annatisch I am using oav version 0.4.38 which will report model validation error. Before I upgraded my oav, I remembered no such model validation error with old version.

@annatisch
Copy link
Member

annatisch commented May 25, 2018

@zhangyd2015 - thanks for the fix - it would be really great if we could get these last 5 examples passing:

  • IntegrationRuntimes_ListByFactory
  • IntegrationRuntimes_Create
  • IntegrationRuntimes_Get
  • IntegrationRuntimes_GetStatus
  • IntegrationRuntimes_Start

Take for example IntegrationRuntimes_Start.

Another common error in the above examples seems to be the use of 'S1' as a value for the 'catalogPricingTier' enum, when the only valid values are "Basic", "Standard", "Premium", "PremiumRS".

@zhangyd2015
Copy link
Contributor Author

@annatisch All the model validation errors have been fixed.

@annatisch
Copy link
Member

Thanks @zhangyd2015 - the PR looks good to me. Once @ravbhatnagar signs off on the new ARM APIs then we're good to go :)

@@ -375,6 +375,99 @@
}
}
},
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.DataFactory/factories/{factoryName}/sharedFactory": {
"post": {
Copy link
Contributor

Choose a reason for hiding this comment

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

The operation name should be an action, it should contain a verb since it is post.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added verb

}
}
},
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.DataFactory/factories/{factoryName}/sharedIntegrationRuntime": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Unless you want to model them as sub resources, then it should be plural. But I prefer action 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.

added verb

@annatisch
Copy link
Member

@ravbhatnagar - are you able to sign-off on the changes made?

@annatisch
Copy link
Member

@hvermis - can you confirm whether your feedback has all been addressed?

@ravbhatnagar
Copy link
Contributor

ravbhatnagar commented Jun 12, 2018

we should be able to close this today. Apologies for the delay here but not an expert on msi so wanted to make sure the right thing is being done here.

@zhangyd2015
Copy link
Contributor Author

@ravbhatnagar I've renamed the API "grantAccess/revokeAccess" as your suggestion.

@ravbhatnagar
Copy link
Contributor

Looks good as per discussion on skype. These are getting added to preview api version. When ARM enables support for listing child resources at a higher scope, these APis will be replaced with that. At that point, there wont be a need for /addIdentity and removeIdentity Apis also. Signing off for now.

@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 Jun 20, 2018
@annatisch
Copy link
Member

Thanks @zhangyd2015 and @ravbhatnagar
There's one further ARM violation in the linter CI:

"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.DataFactory/factories/{factoryName}/integrationRuntimes/{integrationRuntimeName}
has different responses for PUT/GET/PATCH operations. The PUT/GET/PATCH operations must have same schema response."

Is it possible to correct this at this point?

@hvermis
Copy link
Contributor

hvermis commented Jun 20, 2018

@annatisch that violation is a breaking change and we are fixing it in our new API version

@annatisch
Copy link
Member

Thanks @hvermis! @ravbhatnagar - do you approve my adding a suppression for this?

@annatisch
Copy link
Member

ping @ravbhatnagar

@zhangyd2015
Copy link
Contributor Author

@annatisch Since this issue is not a regression introduced by this pull request, can we just merge this pull request and track this issue in separate thread?

@annatisch
Copy link
Member

Thanks @zhangyd2015 - yes I'm happy to do that for the linter error.
However currently CI is failing due to Autorest crashing that I'm trying to diagnose - I will forward you the email conversation.

@hvermis
Copy link
Contributor

hvermis commented Jun 26, 2018

@zhangyd2015 Now that we have new API version out, this change should go there instead of 09-01. Can you please move it?

@zhangyd2015
Copy link
Contributor Author

@annatisch Please hold on the PR merge, I will let you know when we are ready to do that.
Thanks!

@annatisch
Copy link
Member

Sure thing @zhangyd2015.
I had a chat offline and we thing the cause of the broken CI is this line here:
https://github.com/Azure/azure-rest-api-specs/pull/3124/files#diff-bf6d272fbfeeed92d1fc24a2bb8706a8R3218

We think the x-ms-client-flatten extension should be removed here because:

  • This extension should not be combined with polymorphic models.
  • This will be a breaking change for generated SDKs
  • This extension probably shouldn't be used where additional parameters exist alongside the flattened parameters - although we have no firm guidance on this.

Is there a particular reason you chose to flatten these properties? (i.e. the linter tool suggested it).
Otherwise could you try removing it? Hopefully that will fix the build.

@hvermis
Copy link
Contributor

hvermis commented Jun 28, 2018

@annatisch @zhangyd2015 Flattening is added to make authoring easier for our customers. Remove additionalProperties if you have to, as that property is there to keep any future properties that older SDK version doesn't know about, and doesn't help in all cases anyway because of polymorphic limitations.

@zhangyd2015
Copy link
Contributor Author

Thanks @annatisch and @hvermis.

If you select the option "Hide whitespace changes" in "Diff settings", you can find "x-ms-client-flatten" is not introduced in this PR, it should be there for long time.

For additionalProperties, it is not only used for future properties, but also works for list properties. If we remove it, the model validation will be broken somehow.

@hvermis
Copy link
Contributor

hvermis commented Jun 29, 2018

@annatisch, @zhangyd2015 We should not remove flatten directives. Instead remove the additionalProeprties from property classes and this will be fixed. I added those additionalProperties, but they never worked as I wanted.

@annatisch
Copy link
Member

@hvermis - unfortunately in this case the additionalProperties are not the issue. Polymorphic models should not be flattened.

@zhangyd2015
Copy link
Contributor Author

@annatisch, @hvermis, @ravbhatnagar Thanks for the help. We've decided not implementing the preview API, and waiting for the official implementation from ARM level. So I will abandon this PR and create new PR on GA version (version 2018-06-01).

Thank you guys a lot!

@annatisch
Copy link
Member

Thanks @zhangyd2015 - I will close it.
When you submit the PR for the GA version you can reference this one to streamline the review :)

@AutorestCI
Copy link

AutorestCI commented Jul 2, 2018

Automation for azure-sdk-for-java

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-java#2136

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.

5 participants