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

Added go modules for Apis and Client #307

Merged
merged 5 commits into from
Aug 20, 2020

Conversation

Kartik494
Copy link
Member

@Kartik494 Kartik494 commented Apr 27, 2020

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?:

Added Go Module for APIs and Client

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 27, 2020
@k8s-ci-robot
Copy link
Contributor

Welcome @Kartik494!

It looks like this is your first PR to kubernetes-csi/external-snapshotter 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-csi/external-snapshotter has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

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 /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.

@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 Apr 27, 2020
@k8s-ci-robot k8s-ci-robot requested review from lpabon and saad-ali April 27, 2020 11:24
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 27, 2020
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"
Copy link
Collaborator

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member Author

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

@xing-yang
Copy link
Collaborator

/assign @xing-yang

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 5, 2020
@humblec
Copy link
Contributor

humblec commented May 15, 2020

@Kartik494 can you please revisit this PR? please let me know if you need any help on this. I could offer the same.

@Kartik494
Copy link
Member Author

@humblec yes i am working on it

@xing-yang
Copy link
Collaborator

xing-yang commented Jun 22, 2020

@Kartik494 can you take a look of this? https://github.com/kubernetes/klog/blob/master/go.mod
klog supports v2 by adding it to the lib path, but not in the actual code path.
This means dep can't be supported any more, but we probably have to drop it since klog does not support it any more.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2020
@Kartik494 Kartik494 changed the title Added go modules for Apis and Client [WIP] Added go modules for Apis and Client Jun 25, 2020
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 25, 2020
@xing-yang
Copy link
Collaborator

/recheck

apis/go.mod Outdated
@@ -0,0 +1,8 @@
module github.com/kubernetes-csi/external-snapshotter/apis/v2
Copy link
Collaborator

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?

Copy link
Member Author

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

@xing-yang
Copy link
Collaborator

Can you delete the pkg/v2 folder?

@xing-yang
Copy link
Collaborator

Also we need to make sure scripts under pkg/hack still work.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 1, 2020
@Kartik494
Copy link
Member Author

@xing-yang i have pushed a commit, i have move apis under client and remove "V2" folder.Is this the right approach

@xing-yang
Copy link
Collaborator

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jul 11, 2020
@Kartik494
Copy link
Member Author

@xing-yang i have addressed the changes regarding update-crd.sh script file.PTAL


if [ ${SCRIPT_ROOT}/client != $(pwd) ]
then
echo "NOTE: This script should be run from client directory only";
Copy link
Collaborator

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.

@xing-yang
Copy link
Collaborator

xing-yang commented Aug 9, 2020

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.

@xing-yang
Copy link
Collaborator

The script update-generated-code.sh still relies on files under vendor. We should rely on code-generator in the GOPATH instead.

CODEGEN_PKG=${CODEGEN_PKG:-$(cd ${SCRIPT_ROOT}; ls -d -1 ./vendor/k8s.io/code-generator 2>/dev/null || echo ../code-generator)}

@xing-yang
Copy link
Collaborator

xing-yang commented Aug 9, 2020

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

# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

#set -o errexit
set -o nounset
set -o pipefail

SCRIPT_ROOT=$(unset CDPATH && cd $(dirname "${BASH_SOURCE[0]}")/.. && pwd)
echo ${SCRIPT_ROOT}

# find or download controller-gen
CONTROLLER_GEN=$(which controller-gen)

if [ "$CONTROLLER_GEN" = "" ]
then
  TMP_DIR=$(mktemp -d);
  cd $TMP_DIR;
  go mod init tmp;
  go get sigs.k8s.io/controller-tools/cmd/[email protected];
  rm -rf $TMP_DIR;
  CONTROLLER_GEN=$(which controller-gen)
fi

if [ "$CONTROLLER_GEN" = "" ]
then
  echo "ERROR: failed to get controller-gen";
  exit 1;
fi

$CONTROLLER_GEN crd:trivialVersions=true,preserveUnknownFields=false paths=${SCRIPT_ROOT}/apis/volumesnapshot/v1beta1

# To use your own boilerplate text use:
#   --go-header-file ${SCRIPT_ROOT}/hack/custom-boilerplate.go.txt

client/hack/update-generated-code.sh

#
#     http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

set -o errexit
set -o nounset
set -o pipefail

SCRIPT_ROOT=$(unset CDPATH && cd $(dirname "${BASH_SOURCE[0]}")/.. && pwd)
echo ${SCRIPT_ROOT}

# generate the code with:
# --output-base    because this script should also be able to run inside the vendor dir of
#                  k8s.io/kubernetes. The output-base is needed for the generators to output into the vendor dir
#                  instead of the $GOPATH directly. For normal projects this can be dropped.
${GOPATH}/src/k8s.io/code-generator/generate-groups.sh "deepcopy,client,informer,lister" \
  github.com/kubernetes-csi/external-snapshotter/client/v2 github.com/kubernetes-csi/external-snapshotter/client/v2/apis \
  volumesnapshot:v1beta1 \
  --go-header-file ${SCRIPT_ROOT}/hack/boilerplate.go.txt

# To use your own boilerplate text use:
#   --go-header-file ${SCRIPT_ROOT}/hack/custom-boilerplate.go.txt

# Move generated file from client/v2 to client folder and remove client/v2
cp -rf v2/. .
rm -rf v2

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.

@saikat-royc
Copy link
Contributor

Here are some of my observations while trying out the steps pointed by @xing-yang in comment: #307 (comment)

  1. for the v2 directory to be generated in the correct place, we need the external-snapshotter repository to be placed under ~/go/src/github.com/kubernetes-csi/external-snapshotter [Note: if not, then v2 folder gets generated under the following directory
    ~/go/src/sigs.k8s.io/external-snapshotter$ ls github.com/kubernetes-csi/external-snapshotter/client/v2/
    apis]

  2. GOPATH=~/go
    Downloaded the PR and list client dir

~/go/src/github.com/kubernetes-csi/external-snapshotter$ ls -l client/
total 64
drwxr-x--- 3 saikatroyc primarygroup  4096 Aug 10 20:17 apis
drwxr-x--- 3 saikatroyc primarygroup  4096 Aug 10 20:17 clientset
drwxr-x--- 3 saikatroyc primarygroup  4096 Aug 10 20:17 config
-rw-r----- 1 saikatroyc primarygroup   310 Aug 10 20:17 go.mod
-rw-r----- 1 saikatroyc primarygroup 33192 Aug 10 20:17 go.sum
drwxr-x--- 2 saikatroyc primarygroup  4096 Aug 10 20:53 hack
drwxr-x--- 3 saikatroyc primarygroup  4096 Aug 10 20:17 informers
drwxr-x--- 3 saikatroyc primarygroup  4096 Aug 10 20:17 listers
  1. run ./hack/update-generated-code.sh
~/go/src/github.com/kubernetes-csi/external-snapshotter$ ./hack/update-generated-code.sh
Generating deepcopy funcs
Generating clientset for volumesnapshot:v1beta1 at github.com/kubernetes-csi/external-snapshotter/client/clientset
Generating listers for volumesnapshot:v1beta1 at github.com/kubernetes-csi/external-snapshotter/client/listers
Generating informers for volumesnapshot:v1beta1 at github.com/kubernetes-csi/external-snapshotter/client/informers
~/go/src/github.com/kubernetes-csi/external-snapshotter$ ls -l client
total 68
drwxr-x--- 3 saikatroyc primarygroup  4096 Aug 10 20:17 apis
drwxr-x--- 3 saikatroyc primarygroup  4096 Aug 10 20:17 clientset
drwxr-x--- 3 saikatroyc primarygroup  4096 Aug 10 20:17 config
-rw-r----- 1 saikatroyc primarygroup   310 Aug 10 20:17 go.mod
-rw-r----- 1 saikatroyc primarygroup 33192 Aug 10 20:17 go.sum
drwxr-x--- 2 saikatroyc primarygroup  4096 Aug 10 20:53 hack
drwxr-x--- 3 saikatroyc primarygroup  4096 Aug 10 20:17 informers
drwxr-x--- 3 saikatroyc primarygroup  4096 Aug 10 20:17 listers
drwxr-x--- 3 saikatroyc primarygroup  4096 Aug 10 21:35 v2
~/go/src/github.com/kubernetes-csi/external-snapshotter$ ls client/v2/apis/volumesnapshot/v1beta1/zz_generated.deepcopy.go 
client/v2/apis/volumesnapshot/v1beta1/zz_generated.deepcopy.go

@saikat-royc
Copy link
Contributor

saikat-royc commented Aug 10, 2020

For the comment: #307 (comment)

here are the pre-requisites that I performed:

  1. ensure external-snapshotter repo is at ~/go/src/github.com/kubernetes-csi/external-snapshotter
  2. git clone https://github.com/kubernetes/code-generator.git under ~/go/src/k8s.io/code-generator/ and git checkout v0.19.0-rc.2"
  3. GOPATH=~/go
  4. ensure this exists ${GOPATH}/src/k8s.io/code-generator/generate-groups.sh
  5. move hack contents to client directory and run the script (use the modified script posted by xing)
~/go/src/github.com/kubernetes-csi/external-snapshotter$ ./client/hack/update-generated-code.sh

Got the expected results

@xing-yang
Copy link
Collaborator

Thanks @saikat-royc!

@Kartik494, can you take a look of Saikat's comments?

@Kartik494
Copy link
Member Author

Thanks @saikat-royc @xing-yang for the suggestions, i will try the step as mentioned

@Kartik494
Copy link
Member Author

Kartik494 commented Aug 13, 2020

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 go mod tidy command i observe that code-generator is removing from go.mod
diff --git a/go.mod b/go.mod

index efeb2312..16a966b1 100644
--- a/go.mod
+++ b/go.mod
@@ -17,7 +17,6 @@ require (

    k8s.io/api v0.19.0-rc.2
    k8s.io/apimachinery v0.19.0-rc.2
    k8s.io/client-go v0.19.0-rc.2
  •   k8s.io/code-generator v0.19.0-rc.2
      k8s.io/component-base v0.19.0-rc.2
      k8s.io/klog/v2 v2.2.0
    

)

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.

@xing-yang
Copy link
Collaborator

@Kartik494 It is okay that code-generator is removed from go.mod as it is not directly used by the code.

@Kartik494
Copy link
Member Author

/retest

1 similar comment
@Kartik494
Copy link
Member Author

/retest


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.
Copy link
Collaborator

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.


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/
Copy link
Collaborator

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

Copy link
Collaborator

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).
Copy link
Collaborator

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:

  1. Ensure external-snapshotter repo is at ~/go/src/github.com/kubernetes-csi/external-snapshotter
  2. git clone https://github.com/kubernetes/code-generator.git under ~/go/src/k8s.io/code-generator/ and git checkout v0.19.0-rc.2"
  3. GOPATH=~/go
  4. Ensure this exists ${GOPATH}/src/k8s.io/code-generator/generate-groups.sh

@Kartik494
Copy link
Member Author

/retest

@Kartik494
Copy link
Member Author

@xing-yang I have addressed the changes in README also.PTAL

@xing-yang
Copy link
Collaborator

Thanks @Kartik494!

/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 Aug 20, 2020
@k8s-ci-robot
Copy link
Contributor

[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 /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 Aug 20, 2020
@k8s-ci-robot k8s-ci-robot merged commit a69711c into kubernetes-csi:master Aug 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/feature Categorizes issue or PR as related to a new feature. 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 Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants