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

channels for sequences #684

Merged
merged 19 commits into from
Mar 29, 2023
Merged

channels for sequences #684

merged 19 commits into from
Mar 29, 2023

Conversation

johakoch
Copy link
Collaborator

@johakoch johakoch commented Jan 31, 2023

use channels to pass (error) results to and receive results from proxies/requests


Reviewer checklist
  • Read PR description: a summary about the changes is required
  • Changelog updated
  • Documentation: docs/{Reference, Cli, ...}, Docker and cli help/usage
  • Pulled branch, manually tested
  • Verified requirements are met
  • Reviewed the code
  • Reviewed the related tests

@johakoch johakoch force-pushed the channels-for-sequences branch from 9f93643 to 111c18c Compare January 31, 2023 10:14
@johakoch johakoch linked an issue Jan 31, 2023 that may be closed by this pull request
@johakoch johakoch requested a review from malud January 31, 2023 11:10
@johakoch johakoch marked this pull request as ready for review February 1, 2023 08:19
@johakoch
Copy link
Collaborator Author

johakoch commented Feb 1, 2023

I don't think, we need a changelog entry here.
@malud please, have a look.

@malud malud self-assigned this Feb 1, 2023
@johakoch johakoch marked this pull request as draft March 24, 2023 13:14
@johakoch johakoch marked this pull request as ready for review March 24, 2023 13:27
Copy link
Collaborator

@malud malud left a comment

Choose a reason for hiding this comment

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

LGTM, just one more func. Even client cancels seems to work as expected.

config/runtime/endpoint.go Outdated Show resolved Hide resolved
@malud
Copy link
Collaborator

malud commented Mar 28, 2023

Hm unstable tests.

=== RUN   TestJWKsMaxStale
http_integration_test.go:3621: expected status 200, got: 401 ()
RUN   TestOpenAPIValidator_RelativeServerURL
openapi_test.go:138: backend error: status is not supported

Also the openApi test failure I have already seen on this branch.

@johakoch
Copy link
Collaborator Author

johakoch commented Mar 28, 2023

Hm unstable tests.

=== RUN   TestJWKsMaxStale
http_integration_test.go:3621: expected status 200, got: 401 ()
RUN   TestOpenAPIValidator_RelativeServerURL
openapi_test.go:138: backend error: status is not supported

I think, both test failures are unrelated to the changes of this PR.

If I remember correctly, I saw TestJWKsMaxStale failures somewhere else, too.

level=error msg="access control error: stale: received no valid JWKs data: <nil>, status code 500"

Maybe the resource request is sent before the AC has its JWKS information.

TestOpenAPIValidator_RelativeServerURL creates a request to the test backend and passes it to an OpenAPI validating backend. No producers involved.

However, TestOpenAPIValidator_RelativeServerURL could be improved to actually log the entries before the test is stopped by helper.Must(err):

	_, err = backend.RoundTrip(req)
	if err != nil {
		for _, entry := range hook.AllEntries() {
			t.Log(entry.String())
		}
		helper.Must(err)
	}

instead of

	_, err = backend.RoundTrip(req)
	if err != nil {
		helper.Must(err)
	}

	if t.Failed() {
		for _, entry := range hook.AllEntries() {
			t.Log(entry.String())
		}
	}

@malud malud merged commit c3c7c59 into master Mar 29, 2023
@malud malud deleted the channels-for-sequences branch March 29, 2023 06:40
malud pushed a commit that referenced this pull request Oct 13, 2023
* use channels to pass (error) results to and receive results from proxies/requests

* sequence of backend request logs differs, reading log entries is not relevant for 'api-level error handlers affect endpoint's buffer options' test (checking response body is\!); so split tests

* remove unused GetPreviousSequence(); no need to export previousSequence

* rename function, variables

* move adding of independent producers to separate function

---------

Co-authored-by: Marcel Ludwig <1841067+malud@users.noreply.github.com>
Co-authored-by: Alex Schneider <alex.schneider@avenga.com>
Co-authored-by: Marcel Ludwig <marcel.ludwig@milecrew.com>
(cherry picked from commit c3c7c59)
malud added a commit that referenced this pull request Nov 22, 2023
malud added a commit that referenced this pull request Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use channels more consistently for endpoint sequences
2 participants