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

Support setting sticky cookie max age #10176

Merged
merged 5 commits into from Jan 18, 2024
Merged

Support setting sticky cookie max age #10176

merged 5 commits into from Jan 18, 2024

Conversation

Patrick0308
Copy link
Contributor

@Patrick0308 Patrick0308 commented Oct 18, 2023 •
edited by ldez

What does this PR do?

Close #1939

Motivation

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

Related to #10154

@traefiker traefiker added this to the 3.0 milestone Oct 18, 2023
@Patrick0308 Patrick0308 mentioned this pull request Oct 18, 2023
Closed
2 tasks
@Patrick0308
Copy link
Contributor Author

@rtribotte PTAL

@rtribotte
Copy link
Member

Hello @Patrick0308,

Thanks for having opened this new PR.

At a glance, it seems that at least the unit tests (wrr_test.go) remains to be updated.

@Patrick0308
Copy link
Contributor Author

@rtribotte PTAL

@Patrick0308
Copy link
Contributor Author

@rtribotte I don't know why ci failed.

1 similar comment
@Patrick0308

This comment was marked as duplicate.

Sign in to view
@Patrick0308
Copy link
Contributor Author

@rtribotte PTAL

@rtribotte rtribotte self-assigned this Nov 28, 2023
@rtribotte
Copy link
Member

Hello @Patrick0308,

Sure.
I can see that the sticky configuration is not tested in the service package, I suggest adapting the existing tests (service_test.go) to assert the maxAge value.

Also, at least, I can see that the maxAge option is not yet documented in the following places:

@nmengin
Copy link
Contributor

nmengin commented Jan 3, 2024 •
edited

Hello @Patrick0308,

We'd like to bring your feature in Traevik v3.0.
Do you need some help on it?

@Patrick0308
Copy link
Contributor Author

Hello @Patrick0308,

We'd like to bring your feature in Traevik v3.0.

Do you need some help on it?

I haven't started fixing the comment yet. I will work on it on this week.

@nmengin nmengin added the priority/P2 need to be fixed in the future label Jan 8, 2024
@kevinpollet kevinpollet changed the title support setting sticky cookie max age Support setting sticky cookie max age Jan 12, 2024
@rtribotte
Copy link
Member

Hello @Patrick0308,

Thanks for your feedback.
Would you be willing to let us iterate on the PR? We are also perfectly fine letting you take the time to address the comment.

@Patrick0308
Copy link
Contributor Author

Hello @Patrick0308,

Thanks for your feedback. Would you be willing to let us iterate on the PR? We are also perfectly fine letting you take the time to address the comment.

It's a good idea. I'm not good at writing english doc.

@Patrick0308 @kevinpollet
support setting sticky cookie's max-age
8eaf6f4 
Signed-off-by: 白泽 <patrickjiang0530@gmail.com>
Patrick0308 and others added 4 commits January 17, 2024 16:06
@Patrick0308 @kevinpollet
assert maxage
0ee7717 
Signed-off-by: 白泽 <patrickjiang0530@gmail.com>
@Patrick0308 @kevinpollet
modify wrr_test.go
5989ecc 
Signed-off-by: 白泽 <patrickjiang0530@gmail.com>
@Patrick0308 @kevinpollet
max age unit tests
c849b23 
Signed-off-by: 白泽 <patrickjiang0530@gmail.com>
@kevinpollet
review
e79288c
kevinpollet
kevinpollet approved these changes Jan 17, 2024
View reviewed changes
Copy link
Member

@kevinpollet kevinpollet left a comment

Choose a reason for hiding this comment

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

Thanks 👍

rtribotte
rtribotte approved these changes Jan 18, 2024
View reviewed changes
Copy link
Member

@rtribotte rtribotte left a comment

Choose a reason for hiding this comment

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

Thanks 👍

lbenguigui
lbenguigui approved these changes Jan 18, 2024
View reviewed changes
Copy link
Contributor

@lbenguigui lbenguigui left a comment

Choose a reason for hiding this comment

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

LGTM

@traefiker traefiker merged commit 0eeb85d into traefik:v3.0 Jan 18, 2024
22 checks passed
@rtribotte rtribotte removed their assignment Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinpollet kevinpollet kevinpollet approved these changes

@rtribotte rtribotte rtribotte approved these changes

@lbenguigui lbenguigui lbenguigui approved these changes

Assignees
No one assigned
Labels
area/sticky-session kind/enhancement a new or improved feature. priority/P2 need to be fixed in the future size/M
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Patrick0308 @rtribotte @nmengin @kevinpollet @lbenguigui @ldez @traefiker