chore: add all sorts of linters and pre-commit hook #257

zregvart · 2018-12-05T11:56:51Z

This adds a myriad of linters from golangci/golangci-lint[1] and a pre-commit hook to execute them on git commit. Some lint checks needed to be disabled to keep the PR not to big to merge.

To install pre-commit look at the Installation instructions[2] of pre-commit project.

To install golangci-lint follow the Local Installation instructions[3].

[1] https://github.com/golangci/golangci-lint
[2] https://pre-commit.com/#install
[3] https://github.com/golangci/golangci-lint#local-installation

oscerd

LGTM, lets see how much we are able to support the pedantic reports :-)

lburgazzoli · 2018-12-05T12:10:53Z

for what it's worth, I do not like at all installing pre commit hooks :)

One of the reason is that I do commit often, maybe an incomplete work or something I know is not yet ready and having something that do fail to commit hurts me.

It would be better to have this as something we can run with make and/or something we can configure (and eventually disable) in our IDE.

oscerd · 2018-12-05T12:44:46Z

why don't we check directly on CI instead of using pre-commit hooks?

lburgazzoli · 2018-12-05T12:50:50Z

I think the best is to set-up lint on ci and have the option to run the checks locally
Then when and how the checks are running locally is IMHO a matter of preference and personal workflow.

zregvart · 2018-12-05T13:05:02Z

for what it's worth, I do not like at all installing pre commit hooks :)

Then don't add them to your local .git :)

zregvart · 2018-12-05T13:06:04Z

Was --cluster-setup removed?

From CI:

installing camel k cluster resources
Error: unknown flag: --cluster-setup
The command "./build/travis_build.sh" exited with 1.

lburgazzoli · 2018-12-05T13:15:51Z

The pre commit hook config is part of the pr and the docs seems to require to set it up, I know I can skip them but imo it is better to put it as make target and eventually document how to set up hooks as part of the workfkow. Putting the pre commit hook config in the repo has also the drawback that may conflict whit others hook one may have as part of the personal workfkow. Btw I have no objection against lint especially with go but I do not like forcing a specific local workflow

On Wed, 5 Dec 2018 at 14:05, Zoran Regvart ***@***.***> wrote: for what it's worth, I do not like at all installing pre commit hooks :) Then don't add them to your local .git :) — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#257 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AByEhfrmh1rZ52hn_lhnXDsNKTYetiO6ks5u18R-gaJpZM4ZClFf> .

-- -- Luca Burgazzoli

zregvart · 2018-12-05T13:21:34Z

The pre commit hook config is part of the pr and the docs seems to require to set it up, I know I can skip them but imo it is better to put it as make target and eventually document how to set up hooks as part of the workfkow. Putting the pre commit hook config in the repo has also the drawback that may conflict whit others hook one may have as part of the personal workfkow.

I can update the docs to make it clear that installing pre-commit hook is optional and depends on personal preference. Adding .pre-commit-config.yaml should not affect any personal workflow you have, as it is read by the pre-commit hook installed via pre-commit framework.

Don't install pre-commit framework or don't run pre-commit install and your workflow should not be changed.

lburgazzoli · 2018-12-05T13:27:46Z

if one has alteady a pre commit hook configured for camel-k, then the set up put in the repo will conflict with it, correct ?

On Wed, 5 Dec 2018 at 14:21, Zoran Regvart ***@***.***> wrote: The pre commit hook config is part of the pr and the docs seems to require to set it up, I know I can skip them but imo it is better to put it as make target and eventually document how to set up hooks as part of the workfkow. Putting the pre commit hook config in the repo has also the drawback that may conflict whit others hook one may have as part of the personal workfkow. I can update the docs to make it clear that installing pre-commit hook is optional and depends on personal preference. Adding .pre-commit-config.yaml should not affect any personal workflow you have, as it is read by the pre-commit hook installed via pre-commit framework <https://pre-commit.com/>. Don't install pre-commit framework or don't run pre-commit install and your workflow should not be changed. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#257 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AByEhc4UfapsmdLnP5h708spqtDA3BaNks5u18hegaJpZM4ZClFf> .

-- -- Luca Burgazzoli

zregvart · 2018-12-05T13:29:51Z

if one has alteady a pre commit hook configured for camel-k, then the set up put in the repo will conflict with it, correct ?

Yes, if you a) install pre-commit, the framework and b) run pre-commit install.

lburgazzoli · 2018-12-05T13:31:24Z

of course by if I have that file already it means that I have already done a and b :)

zregvart · 2018-12-05T13:40:35Z

of course by if I have that file already it means that I have already done a and b :)

By god man, if you have that file why don't you share it with others?

lburgazzoli · 2018-12-05T13:44:46Z

Again, I may have my own workflow and I don't want to change other people workflow.

Anyway I do not have any problem to leave it in the repo but please add also a make target

...hook This adds a myriad of linters from golangci/golangci-lint[1] and a pre-commit hook to execute them on `git commit`. Some lint checks needed to be disabled to keep the PR not to big to merge. To install pre-commit look at the Installation instructions[2] of pre-commit project. To install golangci-lint follow the Local Installation instructions[3]. [1] https://github.com/golangci/golangci-lint [2] https://pre-commit.com/#install [3] https://github.com/golangci/golangci-lint#local-installation

zregvart · 2018-12-05T14:00:14Z

@lburgazzoli I've added make lint target and clarified the docs.

lburgazzoli · 2018-12-05T14:23:32Z

pkg/client/cmd/install.go

@@ -44,9 +44,12 @@ func newCmdInstall(rootCmdOptions *RootCmdOptions) *cobra.Command {
 	cmd.Flags().StringVar(&options.registry, "registry", "", "A Docker registry that can be used to publish images")
 	cmd.Flags().StringVar(&options.organization, "organization", "", "A organization on the Docker registry that can be used to publish images")
 	cmd.Flags().StringVar(&options.pushSecret, "push-secret", "", "A secret used to push images to the Docker registry")
-	cmd.ParseFlags(os.Args)
+	err := cmd.ParseFlags(os.Args)


@zregvart I think the build problem comes from here: ParseFlags is not needed here, I guess it fails then the error is propagated up to the caller make so the command fails.

Should I just remove it? Lemme see...

lburgazzoli · 2018-12-05T14:54:51Z

pkg/client/cmd/root.go

@@ -57,21 +58,27 @@ func NewKamelCommand(ctx context.Context) (*cobra.Command, error) {
 	cmd.PersistentFlags().StringVarP(&options.Namespace, "namespace", "n", "", "Namespace to use for all operations")

 	// Parse the flags before setting the defaults
-	cmd.ParseFlags(os.Args)
+	err := cmd.ParseFlags(os.Args)


after digging a little bit more this one is also failing, don't know why btw

i guess we should ignore the error for the moment

zregvart · 2018-12-05T15:26:40Z

@lburgazzoli I needed to remove the other ParseFlags in:

camel-k/pkg/client/cmd/root.go

Lines 59 to 60 in 980baca

    
           // Parse the flags before setting the defaults 
        
           cmd.ParseFlags(os.Args)

lburgazzoli · 2018-12-05T15:27:49Z

@zregvart digget into it a little more, I think you can replace the method with something like:

// NewKamelCommand --
func NewKamelCommand(ctx context.Context) (*cobra.Command, error) {
	options := RootCmdOptions{
		Context: ctx,
	}
	var cmd = cobra.Command{
		Use:                    "kamel",
		Short:                  "Kamel is a awesome client tool for running Apache Camel integrations natively on Kubernetes",
		Long:                   kamelCommandLongDescription,
		BashCompletionFunction: bashCompletionFunction,
		PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
			if options.Namespace == "" {
				current, err := kubernetes.GetClientCurrentNamespace(options.KubeConfig)
				if err != nil {
					return errors.Wrap(err, "cannot get current namespace")
				}
				err = cmd.Flag("namespace").Value.Set(current)
				if err != nil {
					return err
				}
			}

			// Let's use a fast refresh period when running with the CLI
			k8sclient.ResetCacheEvery(2 * time.Second)

			// Initialize the Kubernetes client to allow using the operator-sdk
			return kubernetes.InitKubeClient(options.KubeConfig)
		},
	}

	cmd.PersistentFlags().StringVar(&options.KubeConfig, "config", "", "Path to the config file to use for CLI requests")
	cmd.PersistentFlags().StringVarP(&options.Namespace, "namespace", "n", "", "Namespace to use for all operations")

	cmd.AddCommand(newCmdCompletion(&cmd))
	cmd.AddCommand(newCmdVersion())
	cmd.AddCommand(newCmdRun(&options))
	cmd.AddCommand(newCmdGet(&options))
	cmd.AddCommand(newCmdDelete(&options))
	cmd.AddCommand(newCmdInstall(&options))
	cmd.AddCommand(newCmdLog(&options))
	cmd.AddCommand(newCmdContext(&options))

	return &cmd, nil
}

This hooks setting up defaults and initializing the kubernetes client into the cobra lifecycle

zregvart requested review from lburgazzoli, nicolaferraro and oscerd December 5, 2018 11:56

zregvart force-pushed the pr/lint-lint-lint branch from aac9e6b to 4198fde Compare December 5, 2018 12:01

oscerd approved these changes Dec 5, 2018

View reviewed changes

zregvart force-pushed the pr/lint-lint-lint branch from 4198fde to 144e04d Compare December 5, 2018 13:59

lburgazzoli reviewed Dec 5, 2018

View reviewed changes

zregvart force-pushed the pr/lint-lint-lint branch from 3a56d01 to 342aec9 Compare December 5, 2018 15:11

fix: removed cmd.ParseFlags

9274f4c

zregvart force-pushed the pr/lint-lint-lint branch from 342aec9 to 9274f4c Compare December 5, 2018 15:35

lburgazzoli merged commit ee8dee2 into apache:master Dec 5, 2018

lburgazzoli mentioned this pull request Dec 5, 2018

Integrate with GolangCI #258

Closed

zregvart deleted the pr/lint-lint-lint branch December 5, 2018 16:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add all sorts of linters and pre-commit hook #257

chore: add all sorts of linters and pre-commit hook #257

zregvart commented Dec 5, 2018

oscerd left a comment

lburgazzoli commented Dec 5, 2018 •

edited

Loading

oscerd commented Dec 5, 2018

lburgazzoli commented Dec 5, 2018

zregvart commented Dec 5, 2018

zregvart commented Dec 5, 2018

lburgazzoli commented Dec 5, 2018 via email

zregvart commented Dec 5, 2018

lburgazzoli commented Dec 5, 2018 via email

zregvart commented Dec 5, 2018

lburgazzoli commented Dec 5, 2018

zregvart commented Dec 5, 2018

lburgazzoli commented Dec 5, 2018

zregvart commented Dec 5, 2018

lburgazzoli Dec 5, 2018 •

edited

Loading

zregvart Dec 5, 2018

lburgazzoli Dec 5, 2018

lburgazzoli Dec 5, 2018

lburgazzoli Dec 5, 2018

zregvart commented Dec 5, 2018

lburgazzoli commented Dec 5, 2018 •

edited

Loading

chore: add all sorts of linters and pre-commit hook #257

chore: add all sorts of linters and pre-commit hook #257

Conversation

zregvart commented Dec 5, 2018

oscerd left a comment

Choose a reason for hiding this comment

lburgazzoli commented Dec 5, 2018 • edited Loading

oscerd commented Dec 5, 2018

lburgazzoli commented Dec 5, 2018

zregvart commented Dec 5, 2018

zregvart commented Dec 5, 2018

lburgazzoli commented Dec 5, 2018 via email

zregvart commented Dec 5, 2018

lburgazzoli commented Dec 5, 2018 via email

zregvart commented Dec 5, 2018

lburgazzoli commented Dec 5, 2018

zregvart commented Dec 5, 2018

lburgazzoli commented Dec 5, 2018

zregvart commented Dec 5, 2018

lburgazzoli Dec 5, 2018 • edited Loading

Choose a reason for hiding this comment

zregvart Dec 5, 2018

Choose a reason for hiding this comment

lburgazzoli Dec 5, 2018

Choose a reason for hiding this comment

lburgazzoli Dec 5, 2018

Choose a reason for hiding this comment

lburgazzoli Dec 5, 2018

Choose a reason for hiding this comment

zregvart commented Dec 5, 2018

lburgazzoli commented Dec 5, 2018 • edited Loading

lburgazzoli commented Dec 5, 2018 •

edited

Loading

lburgazzoli Dec 5, 2018 •

edited

Loading

lburgazzoli commented Dec 5, 2018 •

edited

Loading