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(cli): report more useful User-Agent on engine API requests #11333

Merged
merged 1 commit into from Jan 11, 2024

Conversation

milas
Copy link
Member

@milas milas commented Jan 10, 2024

What I did
When using the Moby/Docker Engine API client, we do not have a useful user agent value being reported. Ideally, in the future, the Docker CLI will set this appropriately for plugins when it initializes the client.

For now, manually set it, which is a bit hacky because it requires some casting & manually invoking an option function that's technically meant for initialization. In practice, this is pretty safe - the cast is checked defensively and we ignore any errors (which shouldn't be possible anyway).

(not mandatory) A picture of a cute animal, if possible in relation to what you did
two rats snuggling

When using the Moby/Docker Engine API client, we do not have a
useful user agent value being reported. Ideally, in the future,
the Docker CLI will set this appropriately for plugins when it
initializes the client.

For now, manually set it, which is a bit hacky because it
requires some casting & manually invoking an option function
that's technically meant for initialization. In practice, this
is pretty safe - the cast is checked defensively and we ignore
any errors (which shouldn't be possible anyway).

Signed-off-by: Milas Bowman <milas.bowman@docker.com>
Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

codecov bot commented Jan 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1cfeda7) 56.69% compared to head (b621948) 56.66%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11333      +/-   ##
==========================================
- Coverage   56.69%   56.66%   -0.03%     
==========================================
  Files         134      134              
  Lines       11448    11461      +13     
==========================================
+ Hits         6490     6494       +4     
- Misses       4336     4342       +6     
- Partials      622      625       +3     

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

Copy link
Contributor

@ndeloof ndeloof left a comment

Choose a reason for hiding this comment

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

LGTM, but I wonder we are in a hurry to get this and have to introduce such a hack. Can't we just wait for #4574 to be approved ?

@glours
Copy link
Contributor

glours commented Jan 11, 2024

This will allow us to remove hacks on the Docker Desktop side, and we don't know when the next release of Docker CLI with the Milas' PR will be done

@glours glours merged commit dbe7819 into docker:main Jan 11, 2024
26 checks passed
@thaJeztah
Copy link
Member

Looks like this fixes;

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.

None yet

4 participants