-
Notifications
You must be signed in to change notification settings - Fork 542
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
[sos] Set a User Agent for s3 based uploads #3823
base: main
Are you sure you want to change the base?
[sos] Set a User Agent for s3 based uploads #3823
Conversation
4352c29
to
78899d6
Compare
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
Resolves: sosreport#3688 Signed-off-by: Trevor Benson <[email protected]>
78899d6
to
8214a1a
Compare
/packit rebuild-failed |
/packit rebuild-failed |
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.
LGTM
Just a thought, and please excuse my ignorance if it's not applicable... but would it be better to add an option for the command line that allows to set up an s3_user_agent? So the user could specify a user custom agent, use a default one (app/sos), or set None?
It is an option, but it wasn't discussed in the original issue #3688. It may be possible to use AWS_EXECUTION_ENV to achieve this, but I haven't tested it. FWIW, I did not find examples that allow setting the user agent string via a flag in many s3 CLI applications. That is not to say they don't exist; Simply that I did not find them previously when looking. |
@arif-ali rpm-build:fedora-41-ppc64le & rpm-build:fedora-41-s390x still displayed as if they were running 2 weeks after your message to packit. I asked packit to rebuild failures again, but they do not seem to progress. |
@@ -997,10 +998,13 @@ def upload_s3(self, endpoint=None, region=None, bucket=None, prefix=None, | |||
if not secret_key: | |||
secret_key = self.get_upload_s3_secret_key() | |||
|
|||
boto3_config = BotocoreConfig(user_agent_extra='app/sos') |
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 set it to app/sos
because in a later version of boto3 (once Python 3.9 is the minimum supported version) the user_agent_appid="sos"
would append "app/sos" to the user agent. This way in the future if or when sos migrates to using user_agent_appid
the agent string for sos can remain consistent.
I don't thing it matters here much, as it's not based on your change, so all good imho. Maybe someone who knows pack will have better ideas |
imho, user agent is not something that is changed in applications, and it is usually a definition of which application came through. In my opinion, having the option to change would not make sense, but that's my opinion. |
Resolves: #3688
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines
I thought it might be best not to use the bare word
config
orConfig
for just the s3 uploader, so I adjusted them. I'm open to any suggestions, like making them similar (i.e.Boto3Config
orbotocore_config
), or any preferred style suggested.