Add validate cluster command to clusterctl #203

wangzhen127 · 2018-05-23T18:21:08Z

What this PR does / why we need it:
This PR adds validate cluster command to clusterctl. It does not have real implementations yet. This is part of #168 and there will be follow-up PRs.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

NONE

@kubernetes/kube-deploy-reviewers

k8s-ci-robot · 2018-05-23T18:21:22Z

Hi @wangzhen127. Thanks for your PR.

I'm waiting for a kubernetes or kubernetes-sigs 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.

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.

wangzhen127 · 2018-05-23T18:22:37Z

Note that the 1st commit is basically #202. The 2nd commit is the actual PR that will be merged to add validate cluster command.

/cc @karan @jessicaochen

wangzhen127 · 2018-05-23T19:38:22Z

/hold
Should rebase and merge after #202 is merged first.

karan · 2018-05-24T04:41:04Z

I can't review this PR with 11k files. Can you take out the vendor changes and only have the core change in this PR please?

wangzhen127 · 2018-05-24T15:04:32Z

The benefit of including the 1st commit (vendor changes) are:

It gives you the context and why this PR depends on Use i18n for clusterctl command descriptions and update dep #202.
This PR itself is complete and can compile and run. This is important because in terms of dev workflow, I need to check this PR works.
You can still review this PR easily. If you click on the commits tab, you will see 2 commits. The 2nd commit contains the actual changes. There are only 2 files and should be easy to review.

wangzhen127 · 2018-05-24T15:39:30Z

Since I split the commits in #202, I rebased this PR. Now there are 3 commits. You can see them in commits tab.

The first 2 commits come from Use i18n for clusterctl command descriptions and update dep #202, you can review them there.
The 3rd commit is the actual changes in this PR, there are only 2 files and should be easy to review.

karan · 2018-05-25T00:19:46Z

clusterctl/cmd/validate_cluster.go

+}
+
+func init() {
+	validateClusterCmd.Flags().StringVarP(&vco.Name, "name", "n", "", "Cluster name")


Please see jessica's comment here: #119 (comment)

Please leave all flags out of the skeleton. They can be filled in as code uses them. This ensures the the flags actually do something when they appear and also ensure they are defined in a way that works and is accurate to the behavior of the code.

OK. I just removed it. Will add them in follow-up PRs.

wangzhen127 · 2018-05-26T05:26:01Z

Since we decided to drop internalization in #202 for now, I updated this PR to remove those changes. I also updated the PR description. PTAL

/hold cancel

spew · 2018-05-28T19:59:17Z

clusterctl/cmd/validate_cluster.go

+	},
+}
+
+func RunValidateCluster() error {


There should be an associated _test.go file for this file. Please add a validate_cluster_test.go file, it can have a test that calls Run and verifies that errors.NotImplementedError is returned.

spew · 2018-05-28T20:14:17Z

clusterctl/cmd/validate.go

+}
+
+func init() {
+	validateCmd.AddCommand(validateClusterCmd)


To reduce confusion, we should be consistent with who is responsible for adding a sub-command. Since the validate command adds itself to the RootCmd on line 31, then it seems like we should remove line 30 and have the ValidateClusterCmd add itself to the ValidateCmd.

It is not very clear to me which style to follow. createCmd is added in root.go. deleteCmd is added in delete.go instead.

By the way, is there a repo wide style guideline?

As you pointed out there's inconsistency in the repo. We can fix that in a follow up PR. Let's follow the style laid out in the cobra examples [1] where a given command adds itself to its parent. So that means the create command should be added in create.go. And in this case, the validateClusterCommand would add itself to the validate command in validate_cluster.go.

[1] https://github.com/spf13/cobra

FYI opened up a standardization PR here: #228 (comment)

@spew - thanks for standardizing
+1 for following the standardized pattern.

karan

lgtm (will leave the actual "/" approval up to the next reviewer)

karan · 2018-05-29T17:18:41Z

clusterctl/cmd/validate.go

+var validateCmd = &cobra.Command{
+	Use:   "validate",
+	Short: "Validate an API resource created by cluster API.",
+	Long:  "Validate an API resource created by cluster API.",


This can use a little more details. Validate in what way?

I would actually suggest more details on the validate cluster command instead of the validate command as I suspect validation will be cluster API resource-specific.

I agree with Jessica.

And I think it is very early and there is no real implementation yet. I intend to update the help message as I add more stuff in follow-up PRs.

jessicaochen · 2018-05-29T18:21:04Z

clusterctl/cmd/validate.go

+}
+
+func init() {
+	validateCmd.AddCommand(validateClusterCmd)


@spew - thanks for standardizing
+1 for following the standardized pattern.

jessicaochen · 2018-05-29T18:22:12Z

clusterctl/cmd/validate.go

+var validateCmd = &cobra.Command{
+	Use:   "validate",
+	Short: "Validate an API resource created by cluster API.",
+	Long:  "Validate an API resource created by cluster API.",


I would actually suggest more details on the validate cluster command instead of the validate command as I suspect validation will be cluster API resource-specific.

wangzhen127 · 2018-05-29T21:59:09Z

Updated the PR with fixes. PTAL

spew · 2018-05-29T22:08:28Z

/lgtm

spew · 2018-05-29T22:10:51Z

/approve

k8s-ci-robot · 2018-05-29T22:11:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: spew, wangzhen127

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:

~~OWNERS~~ [spew]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

wangzhen127 · 2018-05-29T22:44:32Z

I just noticed that testify was not in vendor/ from the failed test. So I added it in Gopkg.toml and ran dep ensure. Currently I put them as separate commits for easier review. I will squash after someone take a look.

spew · 2018-05-30T13:33:17Z

So to date we have been avoiding testify in this repository and have been relying on writing out if statements. The reasoning comes from the golang FAQ:

https://golang.org/doc/faq#testing_framework

A related point is that testing frameworks tend to develop into mini-languages 
of their own, with conditionals and controls and printing mechanisms, but Go 
already has all those capabilities; why recreate them? We'd rather write tests 
in Go; it's one fewer language to learn and the approach keeps the tests 
straightforward and easy to understand.

I think we need a decision on this and make it a repository mandate or not. Maybe we can conduct this discussion in Slack or have an issue vote.

spew · 2018-05-30T17:11:35Z

/ok-to-test

wangzhen127 · 2018-05-30T18:02:09Z

OK. I updated the test to use if statement. PTAL

spew · 2018-05-30T18:16:09Z

/lgtm

OCPBUGS-30480: Merge https://github.com/kubernetes-sigs/cluster-api:v1.6.4 (36c0e55) into master

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 23, 2018

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 23, 2018

k8s-ci-robot requested review from jessicaochen and karan May 23, 2018 18:22

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 23, 2018

wangzhen127 mentioned this pull request May 24, 2018

Use i18n for clusterctl command descriptions and update dep #202

Closed

wangzhen127 force-pushed the add-validate branch from c12b072 to 5c49119 Compare May 24, 2018 15:35

karan suggested changes May 25, 2018

View reviewed changes

wangzhen127 force-pushed the add-validate branch from 717d157 to 2655acb Compare May 26, 2018 05:22

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 26, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 26, 2018

spew reviewed May 28, 2018

View reviewed changes

karan approved these changes May 29, 2018

View reviewed changes

jessicaochen suggested changes May 29, 2018

View reviewed changes

wangzhen127 force-pushed the add-validate branch from 2655acb to 1b29279 Compare May 29, 2018 21:56

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 29, 2018

k8s-ci-robot assigned spew May 29, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 29, 2018

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 30, 2018

add basic validate cluster command

9831fc1

wangzhen127 force-pushed the add-validate branch from 0a9bb66 to 9831fc1 Compare May 30, 2018 17:59

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 30, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2018

k8s-ci-robot merged commit f69bdf5 into kubernetes-sigs:master May 30, 2018

wangzhen127 deleted the add-validate branch May 31, 2018 17:57

jayunit100 pushed a commit to jayunit100/cluster-api that referenced this pull request Jan 31, 2020

add support for periodic job (kubernetes-sigs#203)

aa46f91

fxierh pushed a commit to fxierh/cluster-api that referenced this pull request Sep 14, 2024

Merge pull request kubernetes-sigs#203 from damdo/rebase-atop-v1.6.4

e9732a4

OCPBUGS-30480: Merge https://github.com/kubernetes-sigs/cluster-api:v1.6.4 (36c0e55) into master

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add validate cluster command to clusterctl #203

Add validate cluster command to clusterctl #203

wangzhen127 commented May 23, 2018 •

edited

Loading

k8s-ci-robot commented May 23, 2018

wangzhen127 commented May 23, 2018

wangzhen127 commented May 23, 2018

karan commented May 24, 2018

wangzhen127 commented May 24, 2018

wangzhen127 commented May 24, 2018

karan May 25, 2018

wangzhen127 May 25, 2018

wangzhen127 commented May 26, 2018

spew May 28, 2018

wangzhen127 May 29, 2018

spew May 28, 2018

wangzhen127 May 28, 2018

wangzhen127 May 29, 2018

spew May 29, 2018

spew May 29, 2018

jessicaochen May 29, 2018

wangzhen127 May 29, 2018

karan left a comment

karan May 29, 2018

jessicaochen May 29, 2018

wangzhen127 May 29, 2018

jessicaochen May 29, 2018

jessicaochen May 29, 2018

wangzhen127 commented May 29, 2018

spew commented May 29, 2018

spew commented May 29, 2018

k8s-ci-robot commented May 29, 2018

wangzhen127 commented May 29, 2018

spew commented May 30, 2018 •

edited

Loading

spew commented May 30, 2018

wangzhen127 commented May 30, 2018

spew commented May 30, 2018

Add validate cluster command to clusterctl #203

Add validate cluster command to clusterctl #203

Conversation

wangzhen127 commented May 23, 2018 • edited Loading

k8s-ci-robot commented May 23, 2018

wangzhen127 commented May 23, 2018

wangzhen127 commented May 23, 2018

karan commented May 24, 2018

wangzhen127 commented May 24, 2018

wangzhen127 commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangzhen127 commented May 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangzhen127 commented May 29, 2018

spew commented May 29, 2018

spew commented May 29, 2018

k8s-ci-robot commented May 29, 2018

wangzhen127 commented May 29, 2018

spew commented May 30, 2018 • edited Loading

spew commented May 30, 2018

wangzhen127 commented May 30, 2018

spew commented May 30, 2018

wangzhen127 commented May 23, 2018 •

edited

Loading

spew commented May 30, 2018 •

edited

Loading