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.

Sign up for GitHub

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

Jump to bottom

Rename unreleased v1 client apis to v1betaX #134

Merged
merged 2 commits into from
May 11, 2021
Merged

Rename unreleased v1 client apis to v1betaX #134

merged 2 commits into from
May 11, 2021

Conversation

mauriciopoppe
Copy link
Member

@mauriciopoppe mauriciopoppe commented May 5, 2021 •
edited
Loading

What type of PR is this?
/kind api-change

What this PR does / why we need it:

Background: https://docs.google.com/document/d/1sBP8f_mwV0N_xRRQQGDwHZpU_nTHtpFxdi7LKSUtgX0/edit#heading=h.bag869lp4lyz

Rename the v1 (unreleased) apis to v1betaX

  • disk, v1 to v1beta3
  • filesystem, v1 to v1beta2
  • smb, v1 to v1beta2
  • volume, v1 to v1beta3

This is preparation work for the refactors in #130, #131, #132 and #133

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 5, 2021
@k8s-ci-robot k8s-ci-robot requested review from andyzhangx and ddebroy May 5, 2021 06:39
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 5, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @mauriciopoppe. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@mauriciopoppe
Copy link
Member Author

/assign @jingxu97 @ddebroy

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 5, 2021
mauriciopoppe
mauriciopoppe commented May 5, 2021
View reviewed changes
integrationtests/volume_test.go
diskv1client "github.com/kubernetes-csi/csi-proxy/client/groups/disk/v1"
v1client "github.com/kubernetes-csi/csi-proxy/client/groups/volume/v1"
v1alpha1client "github.com/kubernetes-csi/csi-proxy/client/groups/volume/v1alpha1"
diskv1beta3 "github.com/kubernetes-csi/csi-proxy/client/api/disk/v1beta3"
Copy link
Member Author

Choose a reason for hiding this comment

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

@jingxu97 @ddebroy please review this file, after reading some part of it it didn't make sense to use alpha apis anymore so I'm only using v1beta apis but I wanted to confirm if this is ok because there were some places where the alpha and the beta client were created explicitly (i.e. it was not a single client per api)

Copy link
Contributor

Choose a reason for hiding this comment

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

The changes LGTM

@mauriciopoppe mauriciopoppe force-pushed the v1-to-v1betaX branch 2 times, most recently from 3296f0a to 6a8f9df Compare May 5, 2021 06:47
pohly added a commit to pohly/csi-proxy that referenced this pull request May 5, 2021
@pohly
Squashed 'release-tools/' changes from 7bc70e5..1748b16
7ce51bd 
1748b16 Merge pull request kubernetes-csi#136 from pohly/go-1.16
ec844ea remove travis.yml, Go 1.16
df76aba Merge pull request kubernetes-csi#134 from andyzhangx/add-build-arg
e314a56 add build-arg ARCH for building multi-arch images, e.g. ARG ARCH FROM k8s.gcr.io/build-image/debian-base-${ARCH}:v2.1.3

git-subtree-dir: release-tools
git-subtree-split: 1748b16b488381c34a86ddbee110e9ed523bcb20
andyzhangx
andyzhangx reviewed May 5, 2021
View reviewed changes
Copy link
Member

@andyzhangx andyzhangx left a comment

Choose a reason for hiding this comment

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

/ok-to-test
Are we downgrading from v1 api to beta? Why?

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 5, 2021
@jingxu97
Copy link
Contributor

jingxu97 commented May 5, 2021

/ok-to-test
Are we downgrading from v1 api to beta? Why?

We went through CSI-proxy API GA review, and got some feedback, there will be quite some API changes. The current V1 (which is not yet released) will not be used.

@msau42
Copy link
Contributor

msau42 commented May 6, 2021

Since we haven't released v1 yet, it's ok to make changes to it.

@mauriciopoppe mauriciopoppe changed the title Rename v1 client apis to v1betaX Rename unreleased v1 client apis to v1betaX May 6, 2021
ddebroy
ddebroy reviewed May 10, 2021
View reviewed changes
Copy link
Contributor

@ddebroy ddebroy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Is it possible to pin the protoc versions for the previous API version so those client go files do not change? (some reference on the prior version used: https://kubernetes.slack.com/archives/CN5JCCW31/p1581058384084900)

Otherwise LGTM! Thanks @mauriciopoppe

client/api/disk/v1alpha1/api.pb.go Outdated
@@ -1,424 +1,693 @@
// Code generated by protoc-gen-go. DO NOT EDIT.
// versions:
Copy link
Contributor

Choose a reason for hiding this comment

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

Will it be possible to not regenerate the protobufs for existing/released API versions?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, I'll revert the changes for other non v1betaX apis

integrationtests/volume_test.go
diskv1client "github.com/kubernetes-csi/csi-proxy/client/groups/disk/v1"
v1client "github.com/kubernetes-csi/csi-proxy/client/groups/volume/v1"
v1alpha1client "github.com/kubernetes-csi/csi-proxy/client/groups/volume/v1alpha1"
diskv1beta3 "github.com/kubernetes-csi/csi-proxy/client/api/disk/v1beta3"
Copy link
Contributor

Choose a reason for hiding this comment

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

The changes LGTM

@jingxu97
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 11, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jingxu97, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 11, 2021
@k8s-ci-robot k8s-ci-robot merged commit fa20ff1 into kubernetes-csi:master May 11, 2021
@mauriciopoppe mauriciopoppe deleted the v1-to-v1betaX branch May 11, 2021 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ddebroy ddebroy ddebroy left review comments

@andyzhangx andyzhangx andyzhangx left review comments

Assignees

@jingxu97 jingxu97

@ddebroy ddebroy

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mauriciopoppe @k8s-ci-robot @jingxu97 @msau42 @andyzhangx @ddebroy