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

add OCI Runtime name to errors #12758

Merged
merged 1 commit into from
Jan 6, 2022

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Jan 6, 2022

It would be easier to diagnose OCI runtime errors if the error actually
had the name of the OCI runtime that produced the error.

[NO NEW TESTS NEEDED]

Signed-off-by: Daniel J Walsh [email protected]

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 6, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

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 Jan 6, 2022
@rhatdan
Copy link
Member Author

rhatdan commented Jan 6, 2022

@mheon @edsantiago @giuseppe PTAL

@mheon
Copy link
Member

mheon commented Jan 6, 2022

Tests are red. Approach LGTM.

@rhatdan
Copy link
Member Author

rhatdan commented Jan 6, 2022

Tests are red because of #12759

It would be easier to diagnose OCI runtime errors if the error actually
had the name of the OCI runtime that produced the error.

[NO NEW TESTS NEEDED]

Signed-off-by: Daniel J Walsh <[email protected]>
Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

One suggestion, but I won't block for it.

LGTM, but I did not sift through the code looking for possible other instances where the runtime could be helpful.

Comment on lines +17 to +18
err_no_such_cmd="Error:.*executable file.* not found in \$PATH: No such file or directory: OCI runtime attempted to invoke a command that was not found"
err_no_exec_dir="Error:.*open executable: Operation not permitted: OCI permission denied"
Copy link
Member

Choose a reason for hiding this comment

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

Why not include the crun: here?

-        err_no_such_cmd="Error:.*executable file.* not found in \$PATH: No such file or directory: OCI runtime attempted to invoke a command that was not found"
-        err_no_exec_dir="Error:.*open executable: Operation not permitted: OCI permission denied"
+        err_no_such_cmd="Error: crun: executable file.* not found in \$PATH: No such file or directory: OCI runtime attempted to invoke a command that was not found"
+        err_no_exec_dir="Error: crun: open executable: Operation not permitted: OCI permission denied"

It seems like a perfect opportunity to confirm that the new code works as expected.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because if others run the tests using other runtimes, then it will blow up.

Copy link
Member

Choose a reason for hiding this comment

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

Look at the line immediately above 😁

Copy link
Member Author

Choose a reason for hiding this comment

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

Missed that.

Copy link
Member

Choose a reason for hiding this comment

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

It's OK. CI is green. I'll add this to my list of minor tweaks to bunch up and commit once in a while.

/lgtm
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 6, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 6, 2022
@edsantiago edsantiago removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 6, 2022
@openshift-merge-robot openshift-merge-robot merged commit 8a22384 into containers:main Jan 6, 2022
edsantiago added a commit to edsantiago/libpod that referenced this pull request Jan 6, 2022
Emergency fix to image-scp tests. DO NOT CREATE A USER!
These tests are run in all sorts of environments. We
do not have the right to vandalize a production system.

Also remove some misleading unneeded tests; and refactor a
little; and add a bunch of FIXMEs which will need to be
addressed later.

Also, super-low priority, add 'crun: ' to expected error
message in a run test (minor followup to containers#12758).

Signed-off-by: Ed Santiago <[email protected]>
@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 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 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.

4 participants