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

[NO TESTS NEEDED] Fix (experimental) Volume Plugin in rootless mode #9685

Conversation

thephoenixofthevoid
Copy link
Contributor

@thephoenixofthevoid thephoenixofthevoid commented Mar 10, 2021

According to #9650 there are few issues when using volume plugins with rootless containers.

The reason behind these is local-driver-specific code being executed for all cases. This is a minimal fix to address these issues.

Please also note that:

  1. In fact, there is a need to clearly separate local-driver-specific code from plugin-related, because there are little to none logic that should be executed in both cases.
  2. The removed check is actually already correctly implemented here
    if volume.config.Driver == define.VolumeDriverLocal {
    logrus.Debugf("Validating options for local driver")
    // Validate options
    for key := range volume.config.Options {
    switch key {
    case "device", "o", "type", "UID", "GID":
    // Do nothing, valid keys
    default:
    return nil, errors.Wrapf(define.ErrInvalidArg, "invalid mount option %s for driver 'local'", key)
    }
    }
    }
  3. These kind of checks is actually implementation specific, so it seems natural to keep them close to the implementation.

@mheon
Copy link
Member

mheon commented Mar 10, 2021

Git validation is complaining that your commits don't have a signoff - git commit --amend -s should add one.

I think it will also complain that you don't have tests. You can add a test (a slightly-changed version (option added) of https://github.com/containers/podman/blob/master/test/e2e/volume_plugin_test.go#L53-L85 would work) or add [NO TESTS NEEDED] to your PR title before you re-push.

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 10, 2021
@thephoenixofthevoid
Copy link
Contributor Author

At this point no new tests are needed, because there is no reliable and stable volume plugin yet. Using unstable volume plugin is rather harmful, as such test is much more likely to break due to unrelated issue, than due to regression of this fix.

@thephoenixofthevoid thephoenixofthevoid changed the title Fix (experimental) Volume Plugin in rootless mode [NO TESTS NEEDED] Fix (experimental) Volume Plugin in rootless mode Mar 11, 2021
@mheon
Copy link
Member

mheon commented Mar 17, 2021

Sorry - it's now complaining about your commit subject length:
- FAIL - commit subject exceeds 90 characters

@mheon
Copy link
Member

mheon commented Mar 23, 2021

This test failure doesn't make any sense. The commit subject it's complaining about doesn't seem to be present in the branch.
@cevich @edsantiago Any ideas?

@edsantiago
Copy link
Member

The second line of a git commit must be empty. @thephoenixofthevoid please insert an empty line between Fix rootless... and In a case of.

@thephoenixofthevoid
Copy link
Contributor Author

Oh, no, it failed again.

@mheon
Copy link
Member

mheon commented Mar 23, 2021

Ah, this one's simple. Just need to rebase against latest master - it's a known test failure due to timezone change that we've fixed already.

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mheon, thephoenixofthevoid

The full list of commands accepted by this bot can be found 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-robot openshift-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 24, 2021
@mheon
Copy link
Member

mheon commented Mar 24, 2021

@thephoenixofthevoid Did you mean to close?

@thephoenixofthevoid
Copy link
Contributor Author

No. It happened as a result of rebase automatically.

@mheon
Copy link
Member

mheon commented Mar 24, 2021

Hm. Git is saying that there are 0 new commits - I think your new commit got lost somehow?

@thephoenixofthevoid
Copy link
Contributor Author

Luckly, I have patch files, and will recreate everything.

@thephoenixofthevoid
Copy link
Contributor Author

#9808

@thephoenixofthevoid
Copy link
Contributor Author

That was completely unexpected, that github closes PR if by chance/by mistake commits get excluded from branch.

@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 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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