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

Updating Functions ARM API specs #7174

Merged
merged 4 commits into from
Oct 18, 2019
Merged

Conversation

mathewc
Copy link
Member

@mathewc mathewc commented Sep 10, 2019

Spec updates for recent Functions ARM API updates. The ARM APIs are live and being used now, and these updates match the current service behavior. This PR replaces a previous PR (#6472).

Notes on the changes:

  • fixed up some description string grammar
  • Added new Types: KeyInfo, HostKeys
  • Added the following properties to existing FunctionEnvelope type: test_data_href, invoke_url_template, language, isDisabled
  • Added the following new APIs:
    • POST api/sites/{name}[/slots/{slot}]/functions/{functionName}/listkeys
    • PUT api/sites/{name}[/slots/{slot}]/functions/{functionName}/keys/{keyName}
    • DELETE api/sites/{name}[/slots/{slot}]/functions/{functionName}/keys/{keyName}
    • POST api/sites/{name}[/slots/{slot}]/host/default/listkeys
    • PUT api/sites/{name}[/slots/{slot}]/host/default/{functionkeys|systemkeys}/{keyName}
    • DELETE api/sites/{name}[/slots/{slot}]/host/default/{functionkeys|systemkeys}/{keyName}
    • POST api/sites/{name}[/slots/{slot}]/host/default/sync
    • POST api/sites/{name}[/slots/{slot}]/host/default/listsyncstatus

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Sep 10, 2019

In Testing, Please Ignore

[Logs] (Generated from b1da089, Iteration 8)

Warning .NET: test-repo-billy/azure-sdk-for-net [Logs] [Diff]
  • No packages generated.
Succeeded Python: test-repo-billy/azure-sdk-for-python [Logs] [Diff]
Failed Java: test-repo-billy/azure-sdk-for-java [Logs] [Diff]
  • Failed appservice/resource-manager/v2016_03_01 [Logs]
  • Failed appservice/resource-manager/v2016_08_01 [Logs]
  • Failed appservice/resource-manager/v2016_09_01 [Logs]
  • Failed appservice/resource-manager/v2018_02_01 [Logs]
Warning Go: test-repo-billy/azure-sdk-for-go [Logs] [Diff]
Failed JavaScript: test-repo-billy/azure-sdk-for-js [Logs] [Diff]
Succeeded Ruby: test-repo-billy/azure-sdk-for-ruby [Logs] [Diff]

@AutorestCI
Copy link

AutorestCI commented Sep 10, 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#8012

@AutorestCI
Copy link

AutorestCI commented Sep 10, 2019

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#6079

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@yungezz yungezz assigned yungezz and unassigned amarzavery Sep 11, 2019
@yungezz yungezz added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Sep 16, 2019
@yungezz
Copy link
Member

yungezz commented Sep 16, 2019

Previous PR is under reviewing #6472. Hi @KrisBash could you pls continue reviewing at the new PR?

@mathewc
Copy link
Member Author

mathewc commented Sep 16, 2019

Thanks guys. As you can see, this PR (and its previous incarnation) have been active for a very long time. If you need any details/clarification from me, I'm more than happy to meet with you guys to discuss. As noted above, this PR maps to the way the service currently behaves. Previous reviewers have pointed out some areas for improvement in response codes etc. but such changes would be breaking and will have to be made in future versions of the APIs.

@lawrencegripper
Copy link

lawrencegripper commented Sep 17, 2019

Hi,

I've done a quick review as based on the emails with Matthew I was pointed to this change as he mentioned the endpoint I was using and fixing in #7143 is now obsolete. The context here is I'm attempting to add the ability to list functions keys into terraform which relies on the autorest generated SDK for Golang which is created from these specs. This is being done based on customer feedback in a compete scenario.

The issue of nested properties and ProxyOnlyResource appears as though it may affect a number of response types for the APIs in 2018-02-01 and 2018-11-01 but I haven't been able to test all of them. I know at the moment that it affects:

  • WebApps_ListFunctionSecrets
  • WebApps_ListHostKeys

I have a PR here addressing these issues for WebApps_ListFunctionSecrets in both 2018-02-01 and 2018-11-01 here: #7155

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.

Please sync with arm api review to discuss.

@KrisBash KrisBash added ARMChangesRequested <valid label in PR review process>add this label when require changes after ARM review and removed ARMReviewInProgress labels Sep 18, 2019
@yungezz
Copy link
Member

yungezz commented Sep 23, 2019

@mathewc could you pls update to address review comments?

@mathewc
Copy link
Member Author

mathewc commented Sep 26, 2019

@KrisBash I addressed feedback and added explanations where needed, thanks.

@mathewc
Copy link
Member Author

mathewc commented Oct 4, 2019

All contentious APIs removed from spec update. @KrisBash I think all issues are addressed now.

@mathewc
Copy link
Member Author

mathewc commented Oct 8, 2019

@KrisBash Ping - we good on this now? Thanks :)

@yungezz
Copy link
Member

yungezz commented Oct 10, 2019

Hi @KrisBash , could you pls review this PR again after updating?

@mathewc
Copy link
Member Author

mathewc commented Oct 11, 2019

@yungezz @KrisBash Anything remaining here or can we merge this?

@mathewc
Copy link
Member Author

mathewc commented Oct 16, 2019

@yungezz @KrisBash Ping

@mathewc mathewc requested a review from KrisBash October 16, 2019 18:16
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.

thanks!

@KrisBash KrisBash added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed ARMChangesRequested <valid label in PR review process>add this label when require changes after ARM review 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 18, 2019

@mathewc is ok to merge now?

@mathewc
Copy link
Member Author

mathewc commented Oct 18, 2019

Yes, please, thanks :)

@lawrencegripper
Copy link

@mathewc and @KrisBash the change has been merged into Terraform and I can confirm that the issues around ListFunctionKeys and ListFunctionSecrets persist.

When using the golang SDK generated from this PR the response object returns nil for all properties. This is a variant of the bug I original reported in the PR.

Screenshot from 2019-11-27 08-48-49

This is because the Spec is incorrect. The response type of stringdictionary doesn't match the rest request.

Spec

// StringDictionary string dictionary resource.
type StringDictionary struct {
	autorest.Response `json:"-"`
	// Properties - Settings.
	Properties map[string]*string `json:"properties"`
	// ID - READ-ONLY; Resource Id.
	ID *string `json:"id,omitempty"`
	// Name - READ-ONLY; Resource Name.
	Name *string `json:"name,omitempty"`
	// Kind - Kind of resource.
	Kind *string `json:"kind,omitempty"`
	// Type - READ-ONLY; Resource type.
	Type *string `json:"type,omitempty"`
}

Actual response

2019/11/27 08:48:10 [DEBUG] AzureRM Response for https://management.azure.com/subscriptions/SOMESUBHERE/resourceGroups/acctestRG-191127084620878438/providers/Microsoft.Web/sites/acctest-SOMEWEBAPP-func/functions/testfunc/listkeys?api-version=2018-02-01: 
HTTP/2.0 200 OK
Cache-Control: no-cache
Content-Type: application/json
Date: Wed, 27 Nov 2019 08:48:09 GMT
Expires: -1
Pragma: no-cache
Server: Microsoft-IIS/10.0
Strict-Transport-Security: max-age=31536000; includeSubDomains
Vary: Accept-Encoding
X-Aspnet-Version: 4.0.30319
X-Content-Type-Options: nosniff
X-Ms-Correlation-Request-Id: 2b01576e-2f24-8b9e-5afc-513858370e02
X-Ms-Ratelimit-Remaining-Subscription-Writes: 1197
X-Ms-Request-Id: 82554e83-d412-4c44-ad33-4322d35aa92e
X-Ms-Routing-Request-Id: UKSOUTH2:20191127T084810Z:d25852f2-eb99-42e6-9b9a-6d252858c434
X-Powered-By: ASP.NET

{"default":"icCCQ97/zFdZ6mXYSOMEKEYHEREpcaBi0X7yI4psRqkI6Ff6e8bA=="}

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.

7 participants