-
Notifications
You must be signed in to change notification settings - Fork 382
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
Added go modules for Apis and Client #307
Conversation
Welcome @Kartik494! |
Hi @Kartik494. 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. |
cmd/csi-snapshotter/main.go
Outdated
clientset "github.com/kubernetes-csi/external-snapshotter/v2/pkg/client/clientset/versioned" | ||
snapshotscheme "github.com/kubernetes-csi/external-snapshotter/v2/pkg/client/clientset/versioned/scheme" | ||
informers "github.com/kubernetes-csi/external-snapshotter/v2/pkg/client/informers/externalversions" | ||
clientset "github.com/kubernetes-csi/external-snapshotter/pkg/client/clientset/versioned" |
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.
For go modules v2 and above, version has to be in the path. See this PR:
#240
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.
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.
@xing-yang thanks, I will update correspondingly
/assign @xing-yang |
@Kartik494 can you please revisit this PR? please let me know if you need any help on this. I could offer the same. |
@humblec yes i am working on it |
@Kartik494 can you take a look of this? https://github.com/kubernetes/klog/blob/master/go.mod |
/recheck |
apis/go.mod
Outdated
@@ -0,0 +1,8 @@ | |||
module github.com/kubernetes-csi/external-snapshotter/apis/v2 |
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.
Can you move "apis" folder under pkg/client and keep them in the same package?
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.
@xing-yang sure i will update this
Can you delete the pkg/v2 folder? |
Also we need to make sure scripts under pkg/hack still work. |
@xing-yang i have pushed a commit, i have move apis under client and remove "V2" folder.Is this the right approach |
/ok-to-test |
@xing-yang i have addressed the changes regarding update-crd.sh script file.PTAL |
hack/update-crd.sh
Outdated
|
||
if [ ${SCRIPT_ROOT}/client != $(pwd) ] | ||
then | ||
echo "NOTE: This script should be run from client directory only"; |
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.
Add a sentence "Refer to hack/README.md" for detailed instructions.
All files, except for go.mod, types.go, etc., under external-snapshotter/client are generated. This means that if I delete all generated files under client and run the scripts, I should get all files generated again. However if I remove all generated files under client, update-generated-code.sh does not work. So this is not working properly. |
The script update-generated-code.sh still relies on files under vendor. We should rely on code-generator in the GOPATH instead.
|
I moved the hack folder to client/hack and made a few changes. I ran all scripts under client now and it works for me. Here are my changes: client/hack/update-crds.sh
client/hack/update-generated-code.sh
Note that I do have ${GOPATH}/src/k8s.io/code-generator and made sure it is using the correct version v0.19.0-rc.2, which is the version referenced in go.mod. This no longer depends on vendor folder. |
Here are some of my observations while trying out the steps pointed by @xing-yang in comment: #307 (comment)
|
For the comment: #307 (comment) here are the pre-requisites that I performed:
Got the expected results |
Thanks @saikat-royc! @Kartik494, can you take a look of Saikat's comments? |
Thanks @saikat-royc @xing-yang for the suggestions, i will try the step as mentioned |
Hi @xing-yang i did changes in my enviorment and follow the steps as mentioned and change the script as you mentioned above.But after performing that when i run index efeb2312..16a966b1 100644
) could you let me know is this the right approach we using in the script "${GOPATH}/src/k8s.io/code-generator/generate-groups.sh " for user to add directory in their enviorment. |
@Kartik494 It is okay that code-generator is removed from go.mod as it is not directly used by the code. |
/retest |
1 similar comment
/retest |
client/hack/README.md
Outdated
|
||
Make sure to run this script after making changes to /client/apis/volumesnapshot/v1beta1/types.go. | ||
|
||
Run: ./hack/update-generated-code.sh from the project root directory. |
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.
Should be running under client now.
client/hack/README.md
Outdated
|
||
Follow these steps to update the CRD: | ||
|
||
* Run ../hack/update-crd.sh from client directory, new yaml files should have been created under ./config/crd/ |
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 path should also be changed:
../hack/update-crd.sh
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.
Please go thru the README and make sure the directory structure referenced in the doc is correct after moving everything under client.
|
||
## update-generated-code.sh | ||
|
||
This is the script to update clientset/informers/listers and API deepcopy code using [code-generator](https://github.com/kubernetes/code-generator). |
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.
Should include these pre-requisites before running update-generated-code.sh:
- Ensure external-snapshotter repo is at ~/go/src/github.com/kubernetes-csi/external-snapshotter
- git clone https://github.com/kubernetes/code-generator.git under ~/go/src/k8s.io/code-generator/ and git checkout v0.19.0-rc.2"
- GOPATH=~/go
- Ensure this exists ${GOPATH}/src/k8s.io/code-generator/generate-groups.sh
/retest |
@xing-yang I have addressed the changes in README also.PTAL |
Thanks @Kartik494! /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Kartik494, xing-yang 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 feature
What this PR does / why we need it:
Added GoModules for API'S AND client
Which issue(s) this PR fixes:
#186
Does this PR introduce a user-facing change?: