-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Register openstack provider for e2e test #74370
Conversation
Since the commit f3d79e1 openstack provider has been denied on e2e test runner. However there are storage e2e tests which are related to openstack. So this adds the registration of openstack provider for e2e test.
/sig openstack |
/cc @dims |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @oomichi , we did remove support for unknown providers in that PR.
/lgtm
/priority important-soon
/approve thanks @oomichi |
@oomichi please work with @mrhillsman and @kiwik to set up a e2e for storage in OpenLab (and report results to testgrid) |
@dims OK, thanks for organizing us. @mrhillsman is helping me for understanding it. |
why are we not just using skeleton or local? |
) | ||
|
||
func init() { | ||
framework.RegisterProvider("openstack", newProvider) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this provider does nothing, this is the same as skeleton
, why would we not just use skeleton?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BenTheElder That is a nice point.
Some storage e2e tests contain cloud provider specific tests like
// OpenStack generic tests (works on all OpenStack deployments) |
If using
skeleton
, we cannot operate these OpenStack specific tests.That is a reason why I proposed this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this hits the long-term planning problem of how to break out providers from the e2e testing framework too. for now i'd say add the nullprovider = openstack back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that also is a nice point. I guess we might need to implement cloud-provider specific e2e tests on each cloud-provider repo in long-term.
I am -1 on this, unless we want to let everyone add their Null provider to the binary we should tell people to use skeleton. |
good point @BenTheElder @oomichi can you please confirm why we need "openstack" registered with a NullProvider? /hold |
indeed.
just modify the test jobs to pass |
To avoid missing my comment, I am putting the same one here: @BenTheElder That is a nice point.
If using skeleton , we cannot operate these OpenStack specific tests.That is a reason why I proposed this PR. |
I see thank you @oomichi that seems like a good reason for now. This will be interesting to figure out long term. |
@BenTheElder Thank you for your reply. |
/hold cancel thanks @oomichi |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dims, oomichi, spiffxp 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 |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Since the commit f3d79e1
openstack provider has been denied on e2e test runner.
However there are storage e2e tests which are related to
openstack. So this adds the registration of openstack
provider for e2e test.
Which issue(s) this PR fixes:
Fixes #74326
Special notes for your reviewer:
This change is tested on my local OpenStack environment.
Does this PR introduce a user-facing change?: