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

feat(relayproxy): allow disable VersionHeader middleware #3141

Merged

Conversation

tomflenner
Copy link
Contributor

@tomflenner tomflenner commented Feb 24, 2025

add a boolean EnableVersionHeader in configuration to conditionnally register the VersionHeader custom middleware and disable the header x-gofeatureflag-version

issue: #3140

Description

Add a new boolean named EnableVersionHeader in documentation with default value set to true in order to not change initial behavior.

The idea behind this is to allow end user to disable VersionHeader custom middleware to remove x-gofeatureflag-version header.

Closes issue(s)

Resolve #3140

Checklist

  • I have tested this code
  • I have added unit test to cover this code
  • I have updated the documentation (README.md and /website/docs)
  • I have followed the contributing guide

Copy link

netlify bot commented Feb 24, 2025

Deploy Preview for go-feature-flag-doc-preview ready!

Name Link
🔨 Latest commit 59e1741
🔍 Latest deploy log https://app.netlify.com/sites/go-feature-flag-doc-preview/deploys/67be03d1e3437b0008a02da1
😎 Deploy Preview https://deploy-preview-3141--go-feature-flag-doc-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Feb 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.86%. Comparing base (7eeeaf1) to head (59e1741).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3141      +/-   ##
==========================================
+ Coverage   84.81%   84.86%   +0.04%     
==========================================
  Files         113      113              
  Lines        5250     5265      +15     
==========================================
+ Hits         4453     4468      +15     
  Misses        632      632              
  Partials      165      165              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tomflenner
Copy link
Contributor Author

tomflenner commented Feb 24, 2025

@thomaspoignant weird for codecov.

I added two test on server_test.go to check that server register and do not register the middleware based on
EnableVersionHeader configuration but it do not recognize it a part of tested line.

Any idea on how to solve this ?

edit: The server do not expose anything to unit test middleware registering

Copy link
Owner

@thomaspoignant thomaspoignant left a comment

Choose a reason for hiding this comment

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

Hey, thanks for your PR.

Considering that the default behavior is to have the middleware activated I would suggest to name the config property DisableVersionHeader instead and also I would try to stick to the common way practice of having Skipper in the middleware instead of a if around, which can allow to activate the middleware on certain patterns.

Also we will nee to update the documentation to mention this new option in this documentation file.

@@ -262,6 +263,10 @@ type Config struct {
// Version is the version of the relay-proxy
Version string `mapstructure:"version" koanf:"version"`

// Enable x-gofeatureflag-version header in the relay-proxy HTTP response
// Default: true
EnableVersionHeader bool `mapstructure:"enableVersionHeader" koanf:"enableversionheader"`
Copy link
Owner

@thomaspoignant thomaspoignant Feb 24, 2025

Choose a reason for hiding this comment

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

🤓 nitpick: ‏Since the default behavior is to have the header enabled, I'll suggest to name this new option VersionHeaderDisable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following other property disableNotifierOnInit, and your first comment #3141 (review) I renamed it DisableVersionHeader

Tom Flenner added 4 commits February 24, 2025 16:31
add a boolean EnableVersionHeader in configuration to conditionnally register the VersionHeader custom middleware and disable the header x-gofeatureflag-version

issue: thomaspoignant#3140
add a boolean DisableVersionHeader in configuration with Skipper on the VersionHeader middleware to

issue: thomaspoignant#3140
@tomflenner tomflenner force-pushed the 3140-config-enable-version-header branch from 51d2592 to f438cd2 Compare February 24, 2025 15:31
Copy link
Owner

@thomaspoignant thomaspoignant left a comment

Choose a reason for hiding this comment

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

@tomflenner Thanks a lot for this addition 🙏

I have updated slightly the PR to be more idiomatic.
I will merge the PR as soon as the CI pass.

Signed-off-by: Thomas Poignant <[email protected]>
@thomaspoignant thomaspoignant force-pushed the 3140-config-enable-version-header branch from 523268e to 31b99c1 Compare February 25, 2025 17:53
@kodiakhq kodiakhq bot merged commit 723b3a0 into thomaspoignant:main Feb 25, 2025
22 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(feature) Disable version header in relay proxy
2 participants