Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
It looks like this was intentional 2dc1af1 (#1381) to address #1353
Is it correct to use the user-agent of the driver? (not the registry acting as client for s3)
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.
I saw those PRs: my takeaway was it wasnt intended for debugging purposes, but that doesn't mean you shouldn't be able to set your own should you wish to do so.
This PR sets it to the "default" value unless the user explicitly requires a specific
User-agent
.Say you run a commercial registry offering and want to distinguish your UA from the default one. How do you imagine doing that? (NOTE: I am not running any commercial service, just imagining a legitimate usecase)
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.
Ah, sorry, I should've looked more in-depth. I wasn't sure where
storageParams["useragent"]
could originate from, and if it could be initialised by the storage-driver itself (i.e., if the storage-driver itself would propagate it with (say) a blanketAmazon s3 client SDK
). If that's not the case, then 💯 on not overriding it if it's set.I'm curious though; is there a global config for this? It looks like it currently is required to be set for the selected storage driver (
config.Storage.Parameters()
returns config options for each driver, which feels ... odd (I would consider the application as a whole to use a specific user-agent)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.
To summarise the conversation I had with Seb offline:
He is suggesting creating a global configuration for
useragent
which can then be passed to individual storage drivers and used anywhere it might be needed.That's a reasonable suggestion and I've made a new issue for it here: #4202