-
-
Notifications
You must be signed in to change notification settings - Fork 649
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 the ability to publish docker images non-interactively. #21880
base: main
Are you sure you want to change the base?
Conversation
e4ef999
to
15b0f80
Compare
Not sure why/how I closed this... Reopening. |
Thanks for contributing, and thanks for waiting for a review. We've just branched for 2.25, so merging this pull request now will come out in 2.26, please move the release notes updates to |
oh and you probably saw this, but Thanks for flying this to the finish line! |
f6bbfc0
to
b677a23
Compare
Ok, tests and docs are updated / fixed |
@benjyw So @trhodeos (whom I worked work -- good luck!) is unlikely to be able to push this forward farther and I'm trying to get it over the line. I've played with it locally, am satisfied that it seems to works, and would happy to merge it and run with it. But I don't have a good sense of the deeper |
Sure, happy to take a look. Can you resolve the conflicts and get a CI going? |
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.
Looks good overall! One substantial design question:
@@ -231,6 +233,24 @@ def env_vars(self) -> tuple[str, ...]: | |||
""" | |||
), | |||
) | |||
publish_noninteractively = BoolOption( |
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.
Why these options are only available on the docker subsystem? Seems like they could apply for any publish
implementation? Docker is the only one that currently uses them in a non-vacuous way, but if that changed I don't think we'd want to add clones of these options in every relevant backend.
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.
My original thought was to have them be per-backend so it'd be clearer where they were supported. Thinking about it now, adding it to the goal and having docs outline which backends support it may make more sense!
Another thought: in a world where many backends support this, not sure if users would want some publish processes (like docker) to be non-interactive while others (say, twine) to be interactive. I have a feeling this'd be "no", but having per-backend options would enable this.
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.
not sure if users would want
Yeah I still have a lot of uncertainty here. Like having docker use this because docker images are big and something else that involves GPG signing not do this something seems reasonable.
I'm not sure what the simplest future proof choice is, or if there is a way to express "non-interactive-able" in a way that doesn't dupe the same options across future subsystems.
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 guess there are two categories of option here. The ones I was thinking should be global (i.e., on PublishSubsystem
) are background_process_output
and publish_noninteractively_verbose
. Since if any publish is noninteractive then these options apply, and for any interactive publish they are vacuous.
The question remains whether publish_noninteractively
itself could similarly be global. It could if we interpret it as "if a publish can be non-interactive then do that, otherwise publish interactively as usual".
Thoughts?
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 I like that more and it does avoid some awkward pass thru dancing. (Done in the latest commit.)
background_process_output
is now in the PublishSubsystem
(and publish_noninteractively_verbose
is correctly deleted all thew way, background_process_output
was intended as ac complete replacement.)
Fixes #17613