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.

Sign up for GitHub

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

Jump to bottom

Remove warning about Logbook bug #10118

Open
wants to merge 9 commits into
base: 4.7.x
Choose a base branch
from
Open

Remove warning about Logbook bug #10118

wants to merge 9 commits into from
+19 −38

Conversation

msdousti
Copy link

The official documentation mentions a major bug for Logbook:

image

It refers to this issue on the Logbook project, which has been fixed by this PR, and integrated into Logbook 3.1.0 on May 19, 2023.

As such, I'd like to suggest removing this waning from the documentation. Thanks!

@CLAassistant
Copy link

CLAassistant commented Nov 14, 2023 •
edited
Loading

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ sdelamo
✅ yawkat
❌ msdousti
You have signed the CLA already but the status is still pending? Let us recheck it.

@sdelamo sdelamo requested a review from yawkat November 14, 2023 17:34
@sdelamo sdelamo added this to the 4.2.1 milestone Nov 14, 2023
sdelamo
sdelamo approved these changes Nov 24, 2023
View reviewed changes
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.

I have updated logbook which we use in the test classpath to 3.6.0

@sdelamo sdelamo marked this pull request as draft November 24, 2023 06:23
@sdelamo
Copy link
Contributor

sdelamo commented Nov 24, 2023

Test def 'h2c do not upgrade fails after upgrading to the latest logbook. @yawkat could you check it out?

@sdelamo sdelamo modified the milestones: 4.2.1, 4.2.2 Nov 24, 2023
@yawkat
Merge branch '4.2.x' into fork/4.2.x
2ec6ea6 
# Conflicts:
#	gradle/libs.versions.toml
@yawkat
Copy link
Member

yawkat commented Dec 6, 2023 •
edited
Loading

ive merged 4.2.x and now the logbook test suite tests fail

@msdousti
Copy link
Author

msdousti commented Dec 6, 2023 •
edited
Loading

The h2c do not upgrade test is failing because of these lines:

micronaut-core/test-suite/src/test/groovy/io/micronaut/docs/netty/LogbookNettyServerCustomizerSpec.groovy

Lines 338 to 339 in 0530ff7

// second response body not included because of logbook bug: https://github.com/zalando/logbook/issues/1216
//'bar',

As explained in the description of the PR, with new Logbook version (3.1 onwards), this bug is resolved, so the "second" bar should now be visible in the logs. I'll adapt the test.

For some other reason, another test tls alpn http 2 started to fail after merging 4.2.x onto this branch. I bumbed Logbook version on 4.2.x itself and the test passed. A bit strange...

PS
The test tls alpn http 2 failed because of the @Requires added in 4fefb40. For the moment, "fixed" by removing it. Please see 6920568.

@msdousti msdousti marked this pull request as ready for review December 6, 2023 16:45
@sdelamo sdelamo modified the milestones: 4.2.2, 4.2.3 Dec 14, 2023
@sdelamo sdelamo modified the milestones: 4.2.3, 4.2.4 Jan 4, 2024
@yawkat
Merge branch 'refs/heads/4.7.x' into fork/msdousti/4.2.x
e296479 
# Conflicts:
#	gradle/libs.versions.toml
#	src/main/docs/guide/httpServer/serverConfiguration/nettyServerPipeline.adoc
#	test-suite/src/test/groovy/io/micronaut/docs/netty/LogbookNettyServerCustomizerSpec.groovy
@yawkat yawkat changed the base branch from 4.2.x to 4.7.x October 7, 2024 12:39
@yawkat
Copy link
Member

yawkat commented Oct 7, 2024

@msdousti can you sign the CLA?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdelamo sdelamo sdelamo approved these changes

@yawkat yawkat Awaiting requested review from yawkat

Assignees
No one assigned
Labels
type: docs
Projects
None yet
Milestone
4.2.4
Development

Successfully merging this pull request may close these issues.
4 participants
@msdousti @CLAassistant @sdelamo @yawkat