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

validate: ignore validation of CDI devices #1241

Closed
wants to merge 1 commit into from

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Nov 22, 2022

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

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 22, 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

@rhatdan
Copy link
Member Author

rhatdan commented Nov 22, 2022

Replaces: #1239

Signed-off-by: Giuseppe Scrivano <[email protected]>
Signed-off-by: Daniel J Walsh <[email protected]>
@rhatdan
Copy link
Member Author

rhatdan commented Nov 22, 2022

@vrothberg @giuseppe PTAL

@giuseppe
Copy link
Member

I am fine with either version, but before taking a decision it can be helpful to look at some numbers first.

I've tried to vendor both versions into Podman, and these are the results I get on Fedora 37 with go1.19.3:

version Podman binary size increment Podman stripped binary size increment
origin/main@1b583a709 48373712 0 34345680 0
origin/main@1b583a709 with PR #1239 48387504 +13792 34350128 +4448
origin/main@1b583a709 with PR #1241 48382768 +9056 34354224 +8544

The version at #1239 adds more lines and increases the value of the binary podman compared to this PR, but the increment is minor after the binary is stripped and we don't have to maintain any new code. I don't think we can easily drop the CDI dependency in Podman since there are more features needed there, like loading the device's description.

@rhatdan
Copy link
Member Author

rhatdan commented Nov 22, 2022

Did you look at Buildah size?

@giuseppe
Copy link
Member

repeating the same check with Buildah, I get:

version Buildah binary size increment Buildah stripped binary size increment
origin/main@c9f30d81ae37 34502784 0 24409488 0
origin/main@c9f30d81ae37 with PR #1239 34513056 +10272 24418448 +8960
origin/main@c9f30d81ae37 with PR #1241 34513848 +11064 24417840 +8352

// of the split components fail to pass syntax validation, vendor and
// class are returned as empty, together with the verbatim input as the
// name and an error describing the reason for failure.
func ParseQualifiedName(device string) (string, string, string, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Name stutters: lint error here.

// If this fails, for instance in the case of unqualified device names,
// ParseDevice returns an empty vendor and class together with name set
// to the verbatim input.
func ParseDevice(device string) (string, string, string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

lint error here

@rhatdan
Copy link
Member Author

rhatdan commented Nov 23, 2022

Ok lets go with original.

@rhatdan rhatdan closed this Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants