Skip to content

Conversation

@baude
Copy link
Member

@baude baude commented Nov 18, 2025

When starting a machine and the user has not explicitly passed -u=true|false AND stdin is a not a tty, we should not prompt to update connections.

Fixes: #27556

Checklist

Ensure you have completed the following checklist for your pull request to be reviewed:

  • Certify you wrote the patch or otherwise have the right to pass it on as an open-source patch by signing all
    commits. (git commit -s). (If needed, use git commit -s --amend). The author email must match
    the sign-off email address. See CONTRIBUTING.md
    for more information.
  • Referenced issues using Fixes: #00000 in commit message (if applicable)
  • Tests have been added/updated (or no tests are needed)
  • Documentation has been updated (or no documentation changes are needed)
  • All commits pass make validatepr (format/lint checks)
  • Release note entered in the section below (or None if no user-facing changes)

Does this PR introduce a user-facing change?

None

@baude baude added No New Tests Allow PR to proceed without adding regression tests and removed release-note-none labels Nov 18, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 18, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 18, 2025
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

btw I think we can add a test for this.
The tests do expose setStdin() so we could set it to a buffer and then after the command runs make sure the buffer was not read.

@packit-as-a-service
Copy link

[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore.

Copy link
Member

@Honny1 Honny1 left a comment

Choose a reason for hiding this comment

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

LGTM, once @Luap99's comments have been addressed.

@baude
Copy link
Member Author

baude commented Nov 18, 2025

btw I think we can add a test for this. The tests do expose setStdin() so we could set it to a buffer and then after the command runs make sure the buffer was not read.

When I did this, the buffer is still being read. I could not pinpoint why (in short order). But the code path is correct and it's going through the right conditionals. So we check the output for the usual prompting message to ensure it is not present.

if err != nil {
return err
}
// This might be kind of lame but when using the command, but if you don't
Copy link
Member

Choose a reason for hiding this comment

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

nit of a nit to be cleaned if you have to repush

Suggested change
// This might be kind of lame but when using the command, but if you don't
// This might be kind of lame but when using the command, if you don't

@TomSweeneyRedHat
Copy link
Member

LGTM once tests and @Luap99 are hip.

When starting a machine and the user has not explicitly passed
-u=true|false AND stdin is a not a tty, we should not prompt to update
connections.

Fixes: containers#27556

Signed-off-by: Brent Baude <bbaude@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. machine No New Tests Allow PR to proceed without adding regression tests release-note-none

Projects

None yet

Development

Successfully merging this pull request may close these issues.

podman machine start update connection should only prompt with tty

4 participants