Skip to content

Conversation

@haircommander
Copy link
Collaborator

@haircommander haircommander commented Jul 22, 2019

Make appropriate changes to varlink exec api and containers_remote to implement remote exec.
Update the exec e2e tests to run with remote exec.

Note, there currently exists one flaw with this implementation, where running a shell in -ti and calling exit causes the runtime to exit with 130 and print an ugly message. not sure why this happens in remote and not local. Stay tuned, but I figured I'd get this out there for review in the meantime
Edit: the above is no longer a problem, and this is ready for review

@haircommander haircommander changed the title Add remote conmon exec Add remote exec Jul 22, 2019
@haircommander haircommander force-pushed the conmon-exec-with-remote-exec branch 2 times, most recently from a269fe0 to 06acabc Compare July 23, 2019 16:49
@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #3630) made this pull request unmergeable. Please resolve the merge conflicts.

including changing -l to the container id
and separating a case of setting the env that remote can't handle

Signed-off-by: Peter Hunt <[email protected]>
There's no way to get the error if we successfully get an exit code (as it's just printed to stderr instead).
instead of relying on the error to be passed to podman, and edit based on the error code, process it on the varlink side instead

Also move error codes to define package

Signed-off-by: Peter Hunt <[email protected]>
Signed-off-by: Peter Hunt <[email protected]>
@haircommander haircommander force-pushed the conmon-exec-with-remote-exec branch from 06acabc to d59f083 Compare July 23, 2019 17:31
@haircommander
Copy link
Collaborator Author

@haircommander haircommander force-pushed the conmon-exec-with-remote-exec branch from 48ae4df to 01a8483 Compare July 23, 2019 20:49
@TomSweeneyRedHat
Copy link
Member

LGTM and happy green tests

@rhatdan
Copy link
Member

rhatdan commented Jul 24, 2019

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, rhatdan

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

The pull request process is described here

Details 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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 24, 2019
@rhatdan
Copy link
Member

rhatdan commented Jul 24, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 24, 2019
@openshift-merge-robot openshift-merge-robot merged commit eae9a00 into containers:master Jul 24, 2019
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants