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

Add default user agent header #613

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

sircpl
Copy link
Contributor

@sircpl sircpl commented Aug 30, 2023

Adds a default user-agent header to requests made by the client. This helps identify traffic coming from the client, but doesn't prevent users from supplying their own user-agent header.

This is slightly modified from the version found in #608. It requires that the version be written into the manifest which is accomplished by addDefaultImplementationEntries

@sircpl sircpl mentioned this pull request Aug 30, 2023
Copy link
Contributor

@aheritier aheritier left a comment

Choose a reason for hiding this comment

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

If it's useful on your side to better identify the different client API and versions let's go

Copy link
Collaborator

@duemir duemir left a comment

Choose a reason for hiding this comment

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

I also agree it is useful. However, I am not sure about the implementation. Instantiating an object to build a sting seems like an overkill to me. I probably would have added package class Headers with USER_AGENT header constant and static method defaultUserAgent. WDYT @PierreBtz ?

Copy link
Collaborator

@PierreBtz PierreBtz left a comment

Choose a reason for hiding this comment

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

@duemir There is additional code needed to retrieve the artifact version. I suggested an alternate approach (#608 (comment)) reading from the MANIFEST (that should be filled by maven at build time IIRC), but it's marginally simpler.

It works as is (tested), so I'm happy, but if you have in mind another simpler approach, I'm interested.

@duemir
Copy link
Collaborator

duemir commented Sep 5, 2023

@PierreBtz It is OK. I'll merge, then.

@duemir duemir merged commit 7d3b4cc into cloudbees-oss:master Sep 5, 2023
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.

4 participants