-
Notifications
You must be signed in to change notification settings - Fork 59
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
Migrate filesystem API Group to library model #231
Migrate filesystem API Group to library model #231
Conversation
Hi @alexander-ding. Thanks for your PR. I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
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.
A lot of these files are showing as new files, I'd move the files in one commit with git mv and then make changes to the imports if needed in another
They're showing up as new files because we're keeping the v1 files around for now. I also think that Git does not track per file history (instead it infers whether a file's been moved by inspecting the diff). I don't think there would be any way around this. |
Run-CSIProxyIntegrationTests -test_args \"--test.v --test.run TestDiskAPIGroup\"; | ||
Run-CSIProxyIntegrationTests -test_args \"--test.v --test.run TestVolumeAPIs\"; | ||
Run-CSIProxyIntegrationTests -test_args \"--test.v --test.run TestSmbAPIGroup\"; | ||
Run-CSIProxyIntegrationTests -test_args \"--test.v\"; |
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.
I think I did this so that I'd get output for each group independently so that one test failure doesn't skip running the other tests
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.
Ah. This is temporary as we migrate since we have two sets of tests, and it's kind of painful to have to keep adding new lines here to do the v2 tests.
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.
I saw this in the logs:
=== RUN TestFilesystem
Which means that these tests are running just fine
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexander-ding, mauriciopoppe 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 api-change
What this PR does / why we need it:
Part of #217 (migrating one API Group to v2's library based model). The v1 model will stick around until all API groups are migrate.
Which issue(s) this PR fixes:
Fixes #226
Special notes for your reviewer:
Do not merge until #230 is merged first.
Does this PR introduce a user-facing change?:
Note that there is no change for now. We don't switch to the new API until the version bump.