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

Azure HTTP Functions compliant with TCK #509

Merged
merged 32 commits into from
Jun 30, 2023
Merged

Azure HTTP Functions compliant with TCK #509

merged 32 commits into from
Jun 30, 2023

Conversation

timyates
Copy link
Contributor

@timyates timyates commented Jun 21, 2023

This PR switches the Azure function handlers to use servlet-core in much the same way as the AWS module does.

Spotted working on this that in a previous PR #398 we broke the test code that we generate in Micronaut Starter (see #509 (comment))

I changed the tests in this module here https://github.com/micronaut-projects/micronaut-azure/pull/509/files#diff-5477701f4559e28df2025cfa073a4ca34eb84685ed452c0567f5c677f040a602

But I am not sure if it's correct...

I have tested this running a test project on Azure, and I think it's right, but it could do with someone who is used to using Azure to check we haven't broken anything important here

I also pulled in the context-path fix from #493 so we don't miss it (or someone else doesn't have to struggle with the merging)

Looking into DefaultExecutionContext to see why using it fails tests in third party apps

@timyates timyates self-assigned this Jun 21, 2023
@timyates
Copy link
Contributor Author

Right, this doesn't work as starter generates code such as:

        HttpRequestMessageBuilder.AzureHttpResponseMessage response =
            function.request(HttpMethod.GET, "/demo")
                    .invoke()

This was broken by #398

@timyates
Copy link
Contributor Author

Tried it in real azure

Returns no content 😭

@timyates
Copy link
Contributor Author

So I think it's an issue with getNativeResponse 🤔

@timyates timyates marked this pull request as ready for review June 23, 2023 13:18
@timyates timyates requested review from sdelamo and wetted June 23, 2023 13:18
@timyates
Copy link
Contributor Author

Raised micronaut-projects/micronaut-starter#1890 to deal with starter generating test code that doesn't work with 4.0.0

Copy link
Contributor

@wetted wetted left a comment

Choose a reason for hiding this comment

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

Looks like a real nice piece of work. Congrats on clearing all the TCK tests.

Copy link
Contributor

@sdelamo sdelamo left a comment

Choose a reason for hiding this comment

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

We need to document you can bind:

com.microsoft.azure.functions.ExecutionContext
com.microsoft.azure.functions.HttpRequestMessage
com.microsoft.azure.functions.TraceContext

Copy link
Contributor

@sdelamo sdelamo left a comment

Choose a reason for hiding this comment

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

It misses documentation about change.

*
*/
@Internal
public final class AzureCookies implements Cookies {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should move this class to io.micronaut.netty.cookies in core.

Copy link
Contributor

Choose a reason for hiding this comment

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

@timyates create an issue for this and add it to 5.1.0 Release project.

@sdelamo sdelamo requested a review from graemerocher June 27, 2023 06:58
@sdelamo sdelamo added the type: enhancement New feature or request label Jun 27, 2023
@sdelamo
Copy link
Contributor

sdelamo commented Jun 27, 2023

@graemerocher can you review this PR? It currently passes every TCK test and I have changed it to tell users they have to remove the azure api route prefix and then they can use normally micronaut.server.context-path if they wish for a prefix.

@sdelamo
Copy link
Contributor

sdelamo commented Jun 27, 2023

We need to document you can bind:

com.microsoft.azure.functions.ExecutionContext com.microsoft.azure.functions.HttpRequestMessage com.microsoft.azure.functions.TraceContext

I have created an issue for this.

* Add TCK tests for http-test

* Add snakeyml -- down to 4 failures

* Fix parameters... 2 to go

* Fixed reader... All fixed

* Remove unused import
@sonarcloud
Copy link

sonarcloud bot commented Jun 27, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
1.4% 1.4% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@sdelamo sdelamo changed the title Rewrite to more closely follow the pattern AWS uses Azure HTTP Functions compliant with TCK Jun 28, 2023
@sdelamo sdelamo merged commit 81e9e18 into master Jun 30, 2023
@sdelamo sdelamo deleted the fix-tck branch June 30, 2023 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
No open projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants