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

Silence hints/recommendations from Docker in the console #1640

Merged
merged 3 commits into from
Feb 14, 2024

Conversation

rmartin16
Copy link
Member

@rmartin16 rmartin16 commented Feb 9, 2024

Changes

Notes

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@rmartin16
Copy link
Member Author

This approach adds a final_kwargs() to Docker and assumes it will be used anytime subprocess is being used to invoke docker. This excludes the docker invocations during verify()...but they could be added if final_kwargs() is made a Class method. I avoided doing this, though, because I'd also like to create a Protocol for all these interfaces that are modeled on Subprocess.

At any rate, another approach I considered was updating dockerize_args() to support invoking docker for commands other than docker run. While that works, it kinda started to overload dockerize_args(), IMO.

Thoughts or other ideas welcome.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

The general approach looks good. My only real hesitation is around the name "final" and whether it's maybe one level of abstraction too many to be worth pulling that out. Yes, it's a repeated configuration; but given that it's only being used in 2 places, that isn't likely to increase (AFAICT), and being an instance method means it can't be easily used for verification... maybe it's just easier to include the extra environment variable in the 2 places it's needed.

@rmartin16
Copy link
Member Author

I agree final_kwargs() may be misleading since this is all still wrapping subprocess. So, what about just an encapsulation for the env for Docker calls to subprocess? This way, it'll be trivial to add additional env vars in the future and it can be added to even the verification docker calls.

@rmartin16 rmartin16 marked this pull request as ready for review February 13, 2024 17:54
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

This approach looks much better to me. One minor nit inline.

# Disable the hints/recommendations that Docker prints in the console
"DOCKER_CLI_HINTS": "false",
}
)
Copy link
Member

Choose a reason for hiding this comment

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

Two minor nits here:

  1. If the user provides an env dictionary as an argument, that dictionary will be modified in place - so using an environment in a subprocess will alter the environment
  2. If, for some reason, you wanted to enable Docker hints (I don't know why, but work with me here 😄 ), there's no way to do so.

What about reversing the direction here: starting with a dictionary that contains `DOCKER_CLI_HINTS: "False"``, and updating that dictionary with env. That will ensure that the final dictionary is a copy, and any explicitly user-provided value will override the default.

Copy link
Member Author

Choose a reason for hiding this comment

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

I should have realized this wouldve updated the dictionary input...which is definitely undesired. I updated it so this (and future) default can be overridden.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

👍

@freakboy3742 freakboy3742 merged commit 7682008 into beeware:main Feb 14, 2024
44 checks passed
@rmartin16 rmartin16 deleted the no-docker-hints branch February 14, 2024 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Silence the recommendations/hints from Docker Desktop
2 participants