-
Notifications
You must be signed in to change notification settings - Fork 113
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
WIP: add option to disable versioning in EOS upon PUT request #4864
base: master
Are you sure you want to change the base?
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
41e988f
to
a1186bd
Compare
…EOS HTTP / GRPC client
b3c36b5
to
6b99f2e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First commit! Thanks.
I've made some comments. Please clean up the commit history to split the feature (and its tests) and all the other things you had to do to be able to do the tests (like updates and registering the gw?).
I'm not able to evaluate if the tests are ok like this. I'll wait for more knowledgeable people.
@@ -0,0 +1,3 @@ | |||
Enhancement: Add HTTP header to disable versioning on EOS | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add 1 paragraph explaining how to use it.
args = append(args, fmt.Sprintf("-ODeos.ruid=%s&eos.rgid=%s&eos.app=%s", auth.Role.UID, auth.Role.GID, app)) | ||
queryParams := fmt.Sprintf("-ODeos.ruid=%s&eos.rgid=%s&eos.app=%s", auth.Role.UID, auth.Role.GID, app) | ||
|
||
if disableVersioning { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this dependent on the auth type? We will switch from gateway-based auth to token-based, so I imagine that would fall under the previous if, and therefore there would be no version disabling possibility?
if disableVersioning { | ||
queryValues.Add("eos.versioning", strconv.Itoa(0)) | ||
} | ||
finalurl := base.String() + queryValues.Encode() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If base
contains query parameters, you are duplicating them.
You might want to base.RawQuery = queryValues.Encode()
and then just finalurl := base.String()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, you don't seem to treat the err
that buildFullURL
might give.
@@ -97,6 +97,13 @@ func NewConn(options Options) (*grpc.ClientConn, error) { | |||
return conn, nil | |||
} | |||
|
|||
func RegisterGatewayServiceClient(client gateway.GatewayAPIClient, endpoint string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explain in the commit message why you had to do this.
Fix for #4855
This PR introduces a new HTTP header for PUT requests,
X-Disable-Versioning
. When this header is added to a PUT request, the EOS storage driver will turn off versioning for this particular save. This is particularly useful for applications that perform automatic saves every x minutes: disabling versioning on these automatic saves ensures that the version history of these files is not polluted