Introduction of a code linting presubmit job

[Radim Hrazdil]

Hello kubevirt-dev,

On a recent call with kubevirt-cares group, we have discussed a need for code linting per each opened PR.

Kubevirt developers work on improving the KubeVirt code quality.
For instance, the recent efforts to substitute pattern like
```
Expect(someList).To(Equal(42)) -> Expect(someList).To(HaveLen(42))
```

, reduce the amount of code in the ambiguous tests/utils.go and other.

However, currently, there is no automated mechanism in place that would prevent mitigated bad code patterns from sneaking back into the codebase.

For that reason, we have proposed a new presubmit lane for code linting:
Job: https://github.com/kubevirt/project-infra/pull/2124
Target: https://github.com/kubevirt/kubevirt/pull/7804

The idea of this job is to run collection of checks to make sure proposed code changes adhere to KubeVirt standards.
In the long term, we could use golangci-lint tool for linting, which seems to be generally accepted in the golang community. It's also relatively easy to write custom plugins for the linter.
In the short term, the checks could be written in bash.

I'll put the job PR on hold for now to give enough time for feedback.

Thanks, Radim

[Roman Mohr]

Hi Radim,

KubeVirt has a code linting in place which runs quite some checks. It is actually not even possible to build KubeVirt without running the code linters defined in [1]. That means that every presubmit lints the code too. 

The build process uses nogo [2]. In [3] you can find an example for a custom plugin which I added.

I would recommend adding any checks this way. It is pretty efficient (runs in parallel to building, so you don't even notice a delay) and you get immediate local feedback and don't have to

wait on CI for some jobs to fail.

I personally would recommend being careful with forcing patterns on this pretty informal level, but if a broad set of people need this and agree to it, automation is definitely the way to flag it. Let's just be sure

that this happens early and always (also local build). I personally find late lint results (after code submit) when writing code pretty annoying.

Best regards,

Roman

[1] https://github.com/kubevirt/kubevirt/blob/main/nogo_config.json

[2] https://github.com/bazelbuild/rules_go/blob/master/go/nogo.rst

[3] https://github.com/kubevirt/kubevirt/pull/4909

 

I'll put the job PR on hold for now to give enough time for feedback.

Thanks, Radim

--
You received this message because you are subscribed to the Google Groups "kubevirt-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubevirt-dev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubevirt-dev/CAKG%3DLOPTjZM5i4gLvg-8exdoGTJzUjxaDRLY3CBXzainmLnzKw%40mail.gmail.com.

[Radim Hrazdil]

Thanks Roman!

I didn't know about the nogo linter.

> I would recommend adding any checks this way. It is pretty efficient (runs in parallel to building, so you don't even notice a delay) and you get immediate local feedback and don't have to

wait on CI for some jobs to fail.

The nogo linter seems to be equally plugginable as the golangci-linter, so we should be good to go for writing any custom analyzers.
One check that @Dan Kenigsberg  specifically asks for however, is to make sure that no new code is added to tests/utils.go, I don't think we can add such a check to any linter.

Could we execute this check during the bazel build as well?

[Roman Mohr]

On Mon, May 30, 2022 at 10:25 AM Radim Hrazdil <rhra...@redhat.com> wrote:

Thanks Roman!

I didn't know about the nogo linter.

> I would recommend adding any checks this way. It is pretty efficient (runs in parallel to building, so you don't even notice a delay) and you get immediate local feedback and don't have to

wait on CI for some jobs to fail.

The nogo linter seems to be equally plugginable as the golangci-linter, so we should be good to go for writing any custom analyzers.
One check that @Dan Kenigsberg  specifically asks for however, is to make sure that no new code is added to tests/utils.go, I don't think we can add such a check to any linter.

Could we execute this check during the bazel build as well?

Yes, the plugins are based on the Analyzer framework from golang [4]. The info passed in contains all kinds of information about the file in general [5]. Most plugins are intended to parset the ast tree of a file, but

like [4] points out there exist  plugins which open the file based on the info in [5] in raw mode and analyze things which go beyond ast tree checking, and that is ok.

Best regards,

Roman

[4] https://pkg.go.dev/golang.org/x/tools/go/analysis#hdr-Analyzer

[5] https://pkg.go.dev/go/ast#File 

 

I'll put the job PR on hold for now to give enough time for feedback.

Thanks, Radim

--
You received this message because you are subscribed to the Google Groups "kubevirt-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubevirt-dev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubevirt-dev/CAKG%3DLOPTjZM5i4gLvg-8exdoGTJzUjxaDRLY3CBXzainmLnzKw%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "kubevirt-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubevirt-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubevirt-dev/CAKG%3DLONtS5kM4sS%2BhYuhgYA_QXTz434cQa3bB0%3DMNJtp7CPaKg%40mail.gmail.com.

[Roman Mohr]

On Mon, May 30, 2022 at 10:38 AM Roman Mohr <rm...@google.com> wrote:

On Mon, May 30, 2022 at 10:25 AM Radim Hrazdil <rhra...@redhat.com> wrote:

Thanks Roman!

I didn't know about the nogo linter.

> I would recommend adding any checks this way. It is pretty efficient (runs in parallel to building, so you don't even notice a delay) and you get immediate local feedback and don't have to

wait on CI for some jobs to fail.

The nogo linter seems to be equally plugginable as the golangci-linter, so we should be good to go for writing any custom analyzers.
One check that @Dan Kenigsberg  specifically asks for however, is to make sure that no new code is added to tests/utils.go, I don't think we can add such a check to any linter.

Could we execute this check during the bazel build as well?

Yes, the plugins are based on the Analyzer framework from golang [4]. The info passed in contains all kinds of information about the file in general [5]. Most plugins are intended to parset the ast tree of a file, but

like [4] points out there exist  plugins which open the file based on the info in [5] in raw mode and analyze things which go beyond ast tree checking, and that is ok.

Oh, and note that the ast expressions should have line number associated. You may be able to just find the last ast tree element and read out the number. But did not check more closely.

That would be super-efficient, sincer every linter gets the pre-created ast-tree for each file passed in, which limits the file-reads in the general case to 1.

[Edward Haas]

I have used golangci-lint in other projects and I have seen it used in many others, with very good results.

It is widely adopted in the golang ecosystem, collecting many linters and allowing a simple way to configure them.

The fact that it comes disconnected from other toolings (e.g. Bazel) is an advantage, as it has a good entry footprint on many small projects.

I can say that when trying to bring in code from kubevirt/kubevirt to other projects, it detected a substantial amount of issues.

I think it is a good start to experiment with.

If bad feedback is received while using it, we can drop it. The hard part is to cleanup the code so it will pass.

KubeVirt has a code linting in place which runs quite some checks. It is actually not even possible to build KubeVirt without running the code linters defined in [1]. That means that every presubmit lints the code too. 

The build process uses nogo [2]. In [3] you can find an example for a custom plugin which I added.

I would recommend adding any checks this way. It is pretty efficient (runs in parallel to building, so you don't even notice a delay) and you get immediate local feedback and don't have to

wait on CI for some jobs to fail.

I personally would recommend being careful with forcing patterns on this pretty informal level, but if a broad set of people need this and agree to it, automation is definitely the way to flag it. Let's just be sure

that this happens early and always (also local build). I personally find late lint results (after code submit) when writing code pretty annoying.

As long as it helps improving the code base and keeping it clean, I am in favor of using all the possible tooling available.

I personally never got blocked by this nogo thing before, nor I saw PR/s I reviewed failing on it.

Whatever the solution is, I think the ability to run it locally should be there no matter what the tool is.

Best regards,

Roman

[1] https://github.com/kubevirt/kubevirt/blob/main/nogo_config.json

[2] https://github.com/bazelbuild/rules_go/blob/master/go/nogo.rst

[3] https://github.com/kubevirt/kubevirt/pull/4909

[4] https://pkg.go.dev/golang.org/x/tools/go/analysis#hdr-Analyzer

[5] https://pkg.go.dev/go/ast#File 

 

I'll put the job PR on hold for now to give enough time for feedback.

Thanks, Radim

--
You received this message because you are subscribed to the Google Groups "kubevirt-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubevirt-dev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubevirt-dev/CAKG%3DLOPTjZM5i4gLvg-8exdoGTJzUjxaDRLY3CBXzainmLnzKw%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "kubevirt-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubevirt-dev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubevirt-dev/CAKG%3DLONtS5kM4sS%2BhYuhgYA_QXTz434cQa3bB0%3DMNJtp7CPaKg%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "kubevirt-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubevirt-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubevirt-dev/CA%2BNAZzy0poZqNu6qfqNo4CMV%2BfFzWBG3%3Dd4EVTMYQgwx3fgKAA%40mail.gmail.com.

[Roman Mohr]

Consider my answers to Radims email as the first round of feedback :)

Roman

 

To view this discussion on the web visit https://groups.google.com/d/msgid/kubevirt-dev/CALmkdFRu-secbEJTuj99Jd9ZE%3DfZ6aM6eW7d2MxPSS-j-gUXZw%40mail.gmail.com.

[Radim Hrazdil]

On Mon, May 30, 2022 at 11:42 AM Roman Mohr <rm...@google.com> wrote:

On Mon, May 30, 2022 at 11:23 AM Edward Haas <edw...@redhat.com> wrote:

On Mon, May 30, 2022 at 11:43 AM 'Roman Mohr' via kubevirt-dev <kubevi...@googlegroups.com> wrote:

On Mon, May 30, 2022 at 10:38 AM Roman Mohr <rm...@google.com> wrote:

On Mon, May 30, 2022 at 10:25 AM Radim Hrazdil <rhra...@redhat.com> wrote:

Thanks Roman!

I didn't know about the nogo linter.

> I would recommend adding any checks this way. It is pretty efficient (runs in parallel to building, so you don't even notice a delay) and you get immediate local feedback and don't have to

wait on CI for some jobs to fail.

The nogo linter seems to be equally plugginable as the golangci-linter, so we should be good to go for writing any custom analyzers.
One check that @Dan Kenigsberg  specifically asks for however, is to make sure that no new code is added to tests/utils.go, I don't think we can add such a check to any linter.

Could we execute this check during the bazel build as well?

Yes, the plugins are based on the Analyzer framework from golang [4]. The info passed in contains all kinds of information about the file in general [5]. Most plugins are intended to parset the ast tree of a file, but

like [4] points out there exist  plugins which open the file based on the info in [5] in raw mode and analyze things which go beyond ast tree checking, and that is ok

Even with the metadata, I'm not sure about checking whether a file grows in the latest PR or not, we would need to keep the size of the file somewhere to have a value to compare.
Also, for this particular check that utils.go doesn't grow, having a separate lane would give a maintainer the opportunity to override it, if there is a good reason to actually put the proposed code to the file. 

Having a lane gives us more flexibility, regardless of the actual linting tool (whether it's golangci-lint or nogo).


 

[Roman Mohr]

Yes, that would not work, but that also does not work from a merge-queue perspective. You will need an optional test-lane then instead of a required check. Since the overrides are only there to allow overriding flakes. Once in the merge-queue the tests will be

executed again (except if your run happened to be on the tip and still is once the PR is ready to merge, which is seldom the case) since, when they are required, they are required to always pass, which leads to consistent behaviour.

If you really want to enforce something like this, for automatic merge-queues, setting a clear max-length for the file is probably the only reasonable way.

You may still run into issues, where your PR worked  initially, then something else got merged, and once you have all approvals, the jobs run again and fail, but then it is pretty much like a merge-conflict and you can reproduce and resolve it against the latest main branch locally.

 

Having a lane gives us more flexibility, regardless of the actual linting tool (whether it's golangci-lint or nogo). 

Both are not really intended to run these  kinds of checks, which include git-history. They are linters which work best when executed at the current merged file. They should give clear signals.

I would not recommend adding any checks which include git history.

Best regards,

Roman

 

 

[Dan Kenigsberg]

On Tue, May 31, 2022 at 10:21 AM Roman Mohr <rm...@google.com> wrote:

On Tue, May 31, 2022 at 8:30 AM Radim Hrazdil <rhra...@redhat.com> wrote:

On Mon, May 30, 2022 at 11:42 AM Roman Mohr <rm...@google.com> wrote:

On Mon, May 30, 2022 at 11:23 AM Edward Haas <edw...@redhat.com> wrote:

On Mon, May 30, 2022 at 11:43 AM 'Roman Mohr' via kubevirt-dev <kubevi...@googlegroups.com> wrote:

On Mon, May 30, 2022 at 10:38 AM Roman Mohr <rm...@google.com> wrote:

On Mon, May 30, 2022 at 10:25 AM Radim Hrazdil <rhra...@redhat.com> wrote:

Thanks Roman!

I didn't know about the nogo linter.

> I would recommend adding any checks this way. It is pretty efficient (runs in parallel to building, so you don't even notice a delay) and you get immediate local feedback and don't have to

wait on CI for some jobs to fail.

The nogo linter seems to be equally plugginable as the golangci-linter, so we should be good to go for writing any custom analyzers.
One check that @Dan Kenigsberg  specifically asks for however, is to make sure that no new code is added to tests/utils.go, I don't think we can add such a check to any linter.

Could we execute this check during the bazel build as well?

Yes, the plugins are based on the Analyzer framework from golang [4]. The info passed in contains all kinds of information about the file in general [5]. Most plugins are intended to parset the ast tree of a file, but

like [4] points out there exist  plugins which open the file based on the info in [5] in raw mode and analyze things which go beyond ast tree checking, and that is ok

Even with the metadata, I'm not sure about checking whether a file grows in the latest PR or not, we would need to keep the size of the file somewhere to have a value to compare.
Also, for this particular check that utils.go doesn't grow, having a separate lane would give a maintainer the opportunity to override it, if there is a good reason to actually put the proposed code to the file.

Yes, that would not work, but that also does not work from a merge-queue perspective. You will need an optional test-lane then instead of a required check. Since the overrides are only there to allow overriding flakes. Once in the merge-queue the tests will be

executed again (except if your run happened to be on the tip and still is once the PR is ready to merge, which is seldom the case) since, when they are required, they are required to always pass, which leads to consistent behaviour.

I'd like to see a lane like Radim's even if it is only optional. If it is made required (which I hope it will), is it a big issue?

We can all agree that tests/utils.go, with its ~4000 lines, defies accepted standards of software engineering. We only need a tool to remind us of this when we propose PRs. I would expect it to be very rare that a PR really needs to add content into it.

But I see your point: while tide merges PRs one by one, an approver would have to keep overriding this commit, just in time. That's what we do when we have a consistent failure, like a broken crio or a faulty sr-iov server, but we don't want to have more of those, right?

If you really want to enforce something like this, for automatic merge-queues, setting a clear max-length for the file is probably the only reasonable way.

You may still run into issues, where your PR worked  initially, then something else got merged, and once you have all approvals, the jobs run again and fail, but then it is pretty much like a merge-conflict and you can reproduce and resolve it against the latest main branch locally.

 

Having a lane gives us more flexibility, regardless of the actual linting tool (whether it's golangci-lint or nogo). 

Both are not really intended to run these kinds of checks, which include git-history. They are linters which work best when executed at the current merged file. They should give clear signals.

I would not recommend adding any checks which include git history.

So should we set a static line check? I volunteer to update it every month or so. This would give us some protection.

But can we do something smarter, that cares only about the PR diff? Maybe a github action?

Regards,

Dan.

[Radim Hrazdil]

I'd like to see a lane like Radim's even if it is only optional. If it is made required (which I hope it will), is it a big issue?

We can all agree that tests/utils.go, with its ~4000 lines, defies accepted standards of software engineering. We only need a tool to remind us of this when we propose PRs. I would expect it to be very rare that a PR really needs to add content into it.

Currently, the lane in the proposal PR is optional, it seems like a good idea to make it optional at least from the beginning to make sure it doesn't misbehave, but I would vote for making it not optional afterwards.

 

But I see your point: while tide merges PRs one by one, an approver would have to keep overriding this commit, just in time. That's what we do when we have a consistent failure, like a broken crio or a faulty sr-iov server, but we don't want to have more of those, right?

If you really want to enforce something like this, for automatic merge-queues, setting a clear max-length for the file is probably the only reasonable way.

You may still run into issues, where your PR worked  initially, then something else got merged, and once you have all approvals, the jobs run again and fail, but then it is pretty much like a merge-conflict and you can reproduce and resolve it against the latest main branch locally.

 

Having a lane gives us more flexibility, regardless of the actual linting tool (whether it's golangci-lint or nogo). 

Both are not really intended to run these kinds of checks, which include git-history. They are linters which work best when executed at the current merged file. They should give clear signals.

I would not recommend adding any checks which include git history.

So should we set a static line check? I volunteer to update it every month or so. This would give us some protection.

If we go for a static check that compares the length of a file with a specified limit, we could introduce a
comment that says how many lines such a file has.
eg:
// \@numberOfLines: 4042

If a PR adds more lines (and exceeds the limit), the author needs to also update the linter comment, which exposes this change to the reviewer.

The linter could also make the check fail if the length doesn't match the comment, so that nobody needs to update the length in the comment, but the price for that would be a bit of annoyance.

[Sarah Bennert ( she / her )]

I would avoid the comment for stated reasons. : )

Adding a gating check to the PR process (github action in this case) will be very helpful in reigning in the expanding files.

A hard limit for all existing files would cause more of a headache.

Instead, should analyze the content of the PR and how it will change the number of lines for each file.

In previous work, we had used a 1000 line length limit per file which was very effective.

The limit utilized should consider the size of the project and severity of the expansion, but should be uniform for all files across a repository.

Each time the gate was triggered, it gave us the opportunity to discuss the benefit of adding to the file and how we could improve our own library through incremental reorganization.

 

 

But can we do something smarter, that cares only about the PR diff? Maybe a github action?

+1 Dan

 

Regards,

Dan.

[Dan Kenigsberg]

On Thu, Jun 2, 2022 at 3:34 PM Sarah Bennert ( she / her ) <sben...@redhat.com> wrote:

On Thursday, June 2, 2022 at 7:03:28 AM UTC-4 rhra...@redhat.com wrote:

I'd like to see a lane like Radim's even if it is only optional. If it is made required (which I hope it will), is it a big issue?

We can all agree that tests/utils.go, with its ~4000 lines, defies accepted standards of software engineering. We only need a tool to remind us of this when we propose PRs. I would expect it to be very rare that a PR really needs to add content into it.

Currently, the lane in the proposal PR is optional, it seems like a good idea to make it optional at least from the beginning to make sure it doesn't misbehave, but I would vote for making it not optional afterwards.

Lacking a better proposal on deck (sorry, I failed to find an example on how to write a simple length-check in the nogo framework), I would love to see your proposed lane merged, and start checking if it misbehaves.

 

But I see your point: while tide merges PRs one by one, an approver would have to keep overriding this commit, just in time. That's what we do when we have a consistent failure, like a broken crio or a faulty sr-iov server, but we don't want to have more of those, right?

If you really want to enforce something like this, for automatic merge-queues, setting a clear max-length for the file is probably the only reasonable way.

You may still run into issues, where your PR worked  initially, then something else got merged, and once you have all approvals, the jobs run again and fail, but then it is pretty much like a merge-conflict and you can reproduce and resolve it against the latest main branch locally.

 

Having a lane gives us more flexibility, regardless of the actual linting tool (whether it's golangci-lint or nogo). 

Both are not really intended to run these kinds of checks, which include git-history. They are linters which work best when executed at the current merged file. They should give clear signals.

I would not recommend adding any checks which include git history.

So should we set a static line check? I volunteer to update it every month or so. This would give us some protection.

If we go for a static check that compares the length of a file with a specified limit, we could introduce a
comment that says how many lines such a file has.
eg:
// \@numberOfLines: 4042

If a PR adds more lines (and exceeds the limit), the author needs to also update the linter comment, which exposes this change to the reviewer.

The linter could also make the check fail if the length doesn't match the comment, so that nobody needs to update the length in the comment, but the price for that would be a bit of annoyance.

I would avoid the comment for stated reasons. : )

I'm not very happy about it either, but in order to get things going I'm proposing a static length check: https://github.com/kubevirt/kubevirt/pull/7852 in line with Radim's suggestion. Please review it if you think, like me, that we should stop dropping code into test/utils.go.

Adding a gating check to the PR process (github action in this case) will be very helpful in reigning in the expanding files.

A hard limit for all existing files would cause more of a headache.

Instead, should analyze the content of the PR and how it will change the number of lines for each file.

In previous work, we had used a 1000 line length limit per file which was very effective.

The limit utilized should consider the size of the project and severity of the expansion, but should be uniform for all files across a repository.

Each time the gate was triggered, it gave us the opportunity to discuss the benefit of adding to the file and how we could improve our own library through incremental reorganization. 

 

 

 

But can we do something smarter, that cares only about the PR diff? Maybe a github action?

+1 Dan

I heard that github actions were once discussed; but there was a preference to have everything under Prow. We can reopen that decision, but I don't know enough about either, so I cannot start that discussion.

 

 

Regards,

Dan.

--
You received this message because you are subscribed to the Google Groups "kubevirt-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubevirt-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubevirt-dev/0cc965a5-ab80-40a3-ac59-08040b560b58n%40googlegroups.com.

[Edward Haas]

On Tue, Jun 7, 2022 at 12:23 PM Dan Kenigsberg <dan...@redhat.com> wrote:

On Thu, Jun 2, 2022 at 3:34 PM Sarah Bennert ( she / her ) <sben...@redhat.com> wrote:

On Thursday, June 2, 2022 at 7:03:28 AM UTC-4 rhra...@redhat.com wrote:

I'd like to see a lane like Radim's even if it is only optional. If it is made required (which I hope it will), is it a big issue?

We can all agree that tests/utils.go, with its ~4000 lines, defies accepted standards of software engineering. We only need a tool to remind us of this when we propose PRs. I would expect it to be very rare that a PR really needs to add content into it.

Currently, the lane in the proposal PR is optional, it seems like a good idea to make it optional at least from the beginning to make sure it doesn't misbehave, but I would vote for making it not optional afterwards.

Lacking a better proposal on deck (sorry, I failed to find an example on how to write a simple length-check in the nogo framework), I would love to see your proposed lane merged, and start checking if it misbehaves.

 

But I see your point: while tide merges PRs one by one, an approver would have to keep overriding this commit, just in time. That's what we do when we have a consistent failure, like a broken crio or a faulty sr-iov server, but we don't want to have more of those, right?

If you really want to enforce something like this, for automatic merge-queues, setting a clear max-length for the file is probably the only reasonable way.

You may still run into issues, where your PR worked  initially, then something else got merged, and once you have all approvals, the jobs run again and fail, but then it is pretty much like a merge-conflict and you can reproduce and resolve it against the latest main branch locally.

 

Having a lane gives us more flexibility, regardless of the actual linting tool (whether it's golangci-lint or nogo). 

Both are not really intended to run these kinds of checks, which include git-history. They are linters which work best when executed at the current merged file. They should give clear signals.

I would not recommend adding any checks which include git history.

So should we set a static line check? I volunteer to update it every month or so. This would give us some protection.

If we go for a static check that compares the length of a file with a specified limit, we could introduce a
comment that says how many lines such a file has.
eg:
// \@numberOfLines: 4042

If a PR adds more lines (and exceeds the limit), the author needs to also update the linter comment, which exposes this change to the reviewer.

The linter could also make the check fail if the length doesn't match the comment, so that nobody needs to update the length in the comment, but the price for that would be a bit of annoyance.

I would avoid the comment for stated reasons. : )

I'm not very happy about it either, but in order to get things going I'm proposing a static length check: https://github.com/kubevirt/kubevirt/pull/7852 in line with Radim's suggestion. Please review it if you think, like me, that we should stop dropping code into test/utils.go.

Now that we have the target, we could bring in the golangci linter and activate exactly this check:
https://golangci-lint.run/usage/linters/#lll

Adding a gating check to the PR process (github action in this case) will be very helpful in reigning in the expanding files.

A hard limit for all existing files would cause more of a headache.

Instead, should analyze the content of the PR and how it will change the number of lines for each file.

In previous work, we had used a 1000 line length limit per file which was very effective.

The limit utilized should consider the size of the project and severity of the expansion, but should be uniform for all files across a repository.

Each time the gate was triggered, it gave us the opportunity to discuss the benefit of adding to the file and how we could improve our own library through incremental reorganization. 

 

 

 

But can we do something smarter, that cares only about the PR diff? Maybe a github action?

+1 Dan

I heard that github actions were once discussed; but there was a preference to have everything under Prow. We can reopen that decision, but I don't know enough about either, so I cannot start that discussion.

 

 

Regards,

Dan.

--
You received this message because you are subscribed to the Google Groups "kubevirt-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubevirt-dev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubevirt-dev/0cc965a5-ab80-40a3-ac59-08040b560b58n%40googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "kubevirt-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubevirt-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubevirt-dev/CAHOEP57v-A98ryWM%2BS-M3ES6j6aTKpTjm1EahFx47BkQdwfZ5A%40mail.gmail.com.

[Dan Kenigsberg]

I'm not sure how this specific linter is configured to limit a certain file's length.

Regardless, since my git-diff-based target failed badly, we reverted to a simple `wc -l`, where override is easy (just loosen the Makefile limit) and no surprises during Tide. Is it time to make this job a required one? We still have 3000 lines too many in tests/utils.

Regards,

Dan.

[Roman Mohr]

Could you summarize for me why we did not just add additional linters to nogo (like the errcheck linter) and did not add a minimal golang-based linter for the file length? nogo and golangci linter both use the same Analyzer interface, so there is not really a difference

regarding to using one or the other, except that we would already have had excellent integration in CI for nogo.

Best regards,

Roman

 

Regards,

Dan.

--
You received this message because you are subscribed to the Google Groups "kubevirt-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubevirt-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubevirt-dev/CAHOEP55z-qTSCXW%3D%2BMwHnwTaPjWshkUxYV-KTw%2B5bUGt-osB1g%40mail.gmail.com.

[Dan Kenigsberg]

My broken git-diff version and Radim's job running it sneaked in before this discussion was concluded; I replaced git-diff with a one-liner bash as a quickfix. Any new job is expensive so we should have waited a bit longer before enabling it.

However, I'm happy that we have it enabled now. I failed to find a rich ecosystem of nogo linters to learn from. So lacking the one-liner, I don't believe anyone would have written a kubevirt nogo linter to block tests/utils from growing further by now. Moreover, it opened the door for enabling more sensible linters from golangci-lint. Having a separate lint job may even allow us to run it on an infrastructure we simply consume, rather than develop and maintain.

[Miguel Duarte de Mora Barroso]

On Wed, Jun 29, 2022 at 3:52 PM Dan Kenigsberg <dan...@redhat.com> wrote:

My broken git-diff version and Radim's job running it sneaked in before this discussion was concluded; I replaced git-diff with a one-liner bash as a quickfix. Any new job is expensive so we should have waited a bit longer before enabling it.

Now that the lint job is enabled, does it make sense to make it **required** ? 

IMO, it doesn't make sense to work on cleaning up the linting errors while contributors inadvertently add more.

If it were a voting member of CI, that wouldn't happen.

 

To view this discussion on the web visit https://groups.google.com/d/msgid/kubevirt-dev/CAHOEP546RaHd-UnPD6nJtLMUbD3oLp3SdFkyB9XW89hqSkkMyQ%40mail.gmail.com.

[Daniel Hiller]

On Wed, Jun 29, 2022 at 4:54 PM Miguel Duarte de Mora Barroso <mdba...@redhat.com> wrote:

On Wed, Jun 29, 2022 at 3:52 PM Dan Kenigsberg <dan...@redhat.com> wrote:

My broken git-diff version and Radim's job running it sneaked in before this discussion was concluded; I replaced git-diff with a one-liner bash as a quickfix. Any new job is expensive so we should have waited a bit longer before enabling it.

Now that the lint job is enabled, does it make sense to make it **required** ? 

I see it failing a lot recently: https://prow.ci.kubevirt.io/job-history/gs/kubevirt-prow/pr-logs/directory/pull-kubevirt-code-lint?buildId=

Didn't look too closely, but seems like timeouts need to get adjusted?

https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_kubevirt/7990/pull-kubevirt-code-lint/1541441473394774016#1:build-log.txt%3A127

So before we make it required we should fix it, otherwise we would risk getting no more merges in.

 

IMO, it doesn't make sense to work on cleaning up the linting errors while contributors inadvertently add more.

Agreed.

 

If it were a voting member of CI, that wouldn't happen. 

Can you elaborate on this? I don't get it.

IMHO, if the majority of people find great value in this then let's take it in. While at the same time I find it odd that we want to have two trails of checks, one being nogo and the other being golangci-lint. But whatever works for the majority of the people is fine with me. But as said in the community meeting, I am not sure whether we have heard enough voices from contributors here.

Note: I can't take any position regarding golangci-lint as I have zero experience with it.

 

To view this discussion on the web visit https://groups.google.com/d/msgid/kubevirt-dev/CAAx_ZVV3zq4DXfQxwScd3j4ougkoEpTJJOoFRaxZ%3DwNx3k%3DU%2BA%40mail.gmail.com.

--

-- 

Best,

Daniel

[Edward Haas]

On Wed, Jun 29, 2022 at 6:18 PM Daniel Hiller <dhi...@redhat.com> wrote:

On Wed, Jun 29, 2022 at 4:54 PM Miguel Duarte de Mora Barroso <mdba...@redhat.com> wrote:

On Wed, Jun 29, 2022 at 3:52 PM Dan Kenigsberg <dan...@redhat.com> wrote:

My broken git-diff version and Radim's job running it sneaked in before this discussion was concluded; I replaced git-diff with a one-liner bash as a quickfix. Any new job is expensive so we should have waited a bit longer before enabling it.

Now that the lint job is enabled, does it make sense to make it **required** ? 

Yes.

I see it failing a lot recently: https://prow.ci.kubevirt.io/job-history/gs/kubevirt-prow/pr-logs/directory/pull-kubevirt-code-lint?buildId=

Didn't look too closely, but seems like timeouts need to get adjusted?

It fails because while some PR/s add coverage, others cause the linter to fail.

This is why Miguel suggested adding the job as required.

It is not related to the timeout, that was fixed already.

https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_kubevirt/7990/pull-kubevirt-code-lint/1541441473394774016#1:build-log.txt%3A127

So before we make it required we should fix it, otherwise we would risk getting no more merges in.

Right. I was waiting for the discussion on https://github.com/kubevirt/kubevirt/pull/7881 to trigger a fix.

I am still waiting.

 

IMO, it doesn't make sense to work on cleaning up the linting errors while contributors inadvertently add more.

Agreed.

 

If it were a voting member of CI, that wouldn't happen. 

Can you elaborate on this? I don't get it.

IMHO, if the majority of people find great value in this then let's take it in. While at the same time I find it odd that we want to have two trails of checks, one being nogo and the other being golangci-lint. But whatever works for the majority of the people is fine with me. But as said in the community meeting, I am not sure whether we have heard enough voices from contributors here.

Note: I can't take any position regarding golangci-lint as I have zero experience with it.

You make it sound as if it is even comparable. If it is, let's compare it.

If there is someone who is willing to contribute Go linters to this nogo and support it, I would like to see a PR and compare the two.

[Roman Mohr]

On Wed, Jun 29, 2022 at 7:24 PM Edward Haas <edw...@redhat.com> wrote:

On Wed, Jun 29, 2022 at 6:18 PM Daniel Hiller <dhi...@redhat.com> wrote:

On Wed, Jun 29, 2022 at 4:54 PM Miguel Duarte de Mora Barroso <mdba...@redhat.com> wrote:

On Wed, Jun 29, 2022 at 3:52 PM Dan Kenigsberg <dan...@redhat.com> wrote:

My broken git-diff version and Radim's job running it sneaked in before this discussion was concluded; I replaced git-diff with a one-liner bash as a quickfix. Any new job is expensive so we should have waited a bit longer before enabling it.

Now that the lint job is enabled, does it make sense to make it **required** ? 

Yes.

I see it failing a lot recently: https://prow.ci.kubevirt.io/job-history/gs/kubevirt-prow/pr-logs/directory/pull-kubevirt-code-lint?buildId=

Didn't look too closely, but seems like timeouts need to get adjusted?

It fails because while some PR/s add coverage, others cause the linter to fail.

This is why Miguel suggested adding the job as required.

It is not related to the timeout, that was fixed already.

https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_kubevirt/7990/pull-kubevirt-code-lint/1541441473394774016#1:build-log.txt%3A127

So before we make it required we should fix it, otherwise we would risk getting no more merges in.

Right. I was waiting for the discussion on https://github.com/kubevirt/kubevirt/pull/7881 to trigger a fix.

I am still waiting.

 

IMO, it doesn't make sense to work on cleaning up the linting errors while contributors inadvertently add more.

Agreed.

 

If it were a voting member of CI, that wouldn't happen. 

Can you elaborate on this? I don't get it.

IMHO, if the majority of people find great value in this then let's take it in. While at the same time I find it odd that we want to have two trails of checks, one being nogo and the other being golangci-lint. But whatever works for the majority of the people is fine with me. But as said in the community meeting, I am not sure whether we have heard enough voices from contributors here.

Note: I can't take any position regarding golangci-lint as I have zero experience with it.

You make it sound as if it is even comparable. If it is, let's compare it.

If there is someone who is willing to contribute Go linters to this nogo and support it, I would like to see a PR and compare the two.

Here Dans longfile linter as nogo analyzer: https://github.com/kubevirt/kubevirt/pull/8016.

As for comparing the two, nogo comes with less built-in linters, but both use the Analyzer interface. I explained that already multiple  times.

For instance to wire errcheck [1], one just needs to be added to the vendor folder (easy by just adding an _ import and running make deps-sync) and [2] added to the nogo linter in the root BUILD.bazel.

While it is slightly more work to initially add them, the advantages are clear and were outline before. Like you most likely hit a lot of linter issues in the path by directly running `make` and just fixed them since you thought that

they were compiler error. Due to the parallelization and caching of bazel it is also super-fast. This is excellent developer experience. I also explained that multiple times.

I think so far you almost always got guidelines from one or more maintainers on how to add your stuff. I think it is up to you to prove that what you want to add is better than the existing flows.

You can't expect that people run off doing all the work for you.

Best regards,

Roman

[1] https://github.com/kisielk/errcheck/errcheck

[2] https://github.com/kisielk/errcheck/blob/master/errcheck/analyzer.go#L13

To view this discussion on the web visit https://groups.google.com/d/msgid/kubevirt-dev/CALmkdFRyoYVFR0MRXHJj5mqiZsnC76OpVPUN_kHt1Q4FFif9NA%40mail.gmail.com.

[Roman Mohr]

On Wed, Jun 29, 2022 at 3:51 PM Dan Kenigsberg <dan...@redhat.com> wrote:

My broken git-diff version and Radim's job running it sneaked in before this discussion was concluded; I replaced git-diff with a one-liner bash as a quickfix. Any new job is expensive so we should have waited a bit longer before enabling it.

However, I'm happy that we have it enabled now. I failed to find a rich ecosystem of nogo linters to learn from. So lacking the one-liner, I don't believe anyone would have written a kubevirt nogo linter to block tests/utils from growing further by now.

If you search "bazel nogo" in google the first result is this link: https://github.com/bazelbuild/rules_go/blob/master/go/nogo.rst, it shows a complete linter implementation and how to register it.

Also note that I immediately shared this link in my first response on this email thread.

I understand that you really have a big issue with the file size and it is ok to make it shorter, but as you know this has lower priority from the maintainer side, so no one of us took this task.

If you really wish to have it, I would expect that you either spend the time on the research or convince someone to do it for you. If both things do not happen it may be ok to not have the check due to the obvious low priority then, instead of just moving forward and making the normal developer flow more difficult.

It looks like that you mostly spend a little bit of time on superficial improvements on the project without real intent to develop any features on kubevirt. This is completely fine and also welcome, however you may not

be in a position then to understand the pros of the current setup.

Developer experience (when not doing release engineering) has a pretty high standard at kubevirt, which makes release engineering tasks more complex and one has to reserve the time for that.

Best regards,

Roman

To view this discussion on the web visit https://groups.google.com/d/msgid/kubevirt-dev/CAHOEP546RaHd-UnPD6nJtLMUbD3oLp3SdFkyB9XW89hqSkkMyQ%40mail.gmail.com.

[Daniel Hiller]

On Thu, Jun 30, 2022 at 9:30 AM 'Roman Mohr' via kubevirt-dev <kubevi...@googlegroups.com> wrote:

On Wed, Jun 29, 2022 at 3:51 PM Dan Kenigsberg <dan...@redhat.com> wrote:

My broken git-diff version and Radim's job running it sneaked in before this discussion was concluded; I replaced git-diff with a one-liner bash as a quickfix. Any new job is expensive so we should have waited a bit longer before enabling it.

However, I'm happy that we have it enabled now. I failed to find a rich ecosystem of nogo linters to learn from. So lacking the one-liner, I don't believe anyone would have written a kubevirt nogo linter to block tests/utils from growing further by now.

If you search "bazel nogo" in google the first result is this link: https://github.com/bazelbuild/rules_go/blob/master/go/nogo.rst, it shows a complete linter implementation and how to register it.

Also note that I immediately shared this link in my first response on this email thread.

I understand that you really have a big issue with the file size and it is ok to make it shorter, but as you know this has lower priority from the maintainer side, so no one of us took this task.

If you really wish to have it, I would expect that you either spend the time on the research or convince someone to do it for you. If both things do not happen it may be ok to not have the check due to the obvious low priority then, instead of just moving forward and making the normal developer flow more difficult.

It looks like that you mostly spend a little bit of time on superficial improvements on the project without real intent to develop any features on kubevirt. This is completely fine and also welcome, however you may not

be in a position then to understand the pros of the current setup.

Developer experience (when not doing release engineering) has a pretty high standard at kubevirt, which makes release engineering tasks more complex and one has to reserve the time for that.

So, moving forward, wouldn't the best solution be to include all the wanted golangci-lint linters into nogo, as this should be easy to do, since they are implementing the analyzer interface and can be picked up by nogo? And also include the nogo filelength stuff from Roman. And as a last step removing the separate lint make target?

 

To view this discussion on the web visit https://groups.google.com/d/msgid/kubevirt-dev/CA%2BNAZzx2EmBWxFt8eurTobdgo3e0zF1h8HBCrAmAANQDX8Qw3g%40mail.gmail.com.

--

-- 

Best,

Daniel

[Miguel Duarte de Mora Barroso]

On Thu, Jun 30, 2022 at 2:28 PM Daniel Hiller <dhi...@redhat.com> wrote:

On Thu, Jun 30, 2022 at 9:30 AM 'Roman Mohr' via kubevirt-dev <kubevi...@googlegroups.com> wrote:

On Wed, Jun 29, 2022 at 3:51 PM Dan Kenigsberg <dan...@redhat.com> wrote:

My broken git-diff version and Radim's job running it sneaked in before this discussion was concluded; I replaced git-diff with a one-liner bash as a quickfix. Any new job is expensive so we should have waited a bit longer before enabling it.

However, I'm happy that we have it enabled now. I failed to find a rich ecosystem of nogo linters to learn from. So lacking the one-liner, I don't believe anyone would have written a kubevirt nogo linter to block tests/utils from growing further by now.

If you search "bazel nogo" in google the first result is this link: https://github.com/bazelbuild/rules_go/blob/master/go/nogo.rst, it shows a complete linter implementation and how to register it.

Also note that I immediately shared this link in my first response on this email thread.

I understand that you really have a big issue with the file size and it is ok to make it shorter, but as you know this has lower priority from the maintainer side, so no one of us took this task.

If you really wish to have it, I would expect that you either spend the time on the research or convince someone to do it for you. If both things do not happen it may be ok to not have the check due to the obvious low priority then, instead of just moving forward and making the normal developer flow more difficult.

It looks like that you mostly spend a little bit of time on superficial improvements on the project without real intent to develop any features on kubevirt. This is completely fine and also welcome, however you may not

be in a position then to understand the pros of the current setup.

Developer experience (when not doing release engineering) has a pretty high standard at kubevirt, which makes release engineering tasks more complex and one has to reserve the time for that.

So, moving forward, wouldn't the best solution be to include all the wanted golangci-lint linters into nogo, as this should be easy to do, since they are implementing the analyzer interface and can be picked up by nogo? And also include the nogo filelength stuff from Roman. And as a last step removing the separate lint make target?

While it is now clear to me the developer experience provided by the nogo integrated linters is (quite) better, it is also clear that it takes more time to integrate them. 

... and there are plenty of them.

 

The question - imho - is how long would it take a developer (or a series of developers) to introduce those linters one by one into nogo ? Is that worth the cost ? Honest question.

Take for instance Roman's work in bringing ineffassign into the project - it took from Jan 27, 2021 to Feb 13th 2021 to be integrated - yes, I realize we had to wait for a change

to be made to the linter's source code.

The current alternative, which works across the golang's ecosystem seamlessly, is available now - *but* provides a weaker dev experience.

The metric we should try to optimize is of course, code quality. But if the end result is the same, we reach the conundrum of engineering time + more code to maintain vs worse dev. experience. I tend to favor the solution that costs less in terms of engineering time, but that's just me.

Anyway, I fully agree that if we're not making the golang-ci linters required, they have no place in the code, and should be removed. 

 

I'm actually even willing to try out integrating one of these linters into no-go once I can spare some time.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubevirt-dev/CAK%2BeyL5vL2%2B9NnMZ5gnGyJzTdJOTWtxvrHMgnSy%3Dqq8hSa6SWw%40mail.gmail.com.

[Itamar Holder]

I think excellent questions were raised here that are important to discuss, and we haven't yet reached a solid conclusion.

To summarize, these are (in my eyes) the open questions that remain:

1) How easy/hard is it to add more linters into nogo?

On the one hand Roman said that it's "easy by just adding an _ import and running make deps-sync". On the other hand, as Miguel pointed out, the PR that introduced that was very long and complex and took time to merge. Even the PR for checking file length requires its fair amount of development work / review cycle, etc. I also want to point out that the fact is that basically no one added any linter to nogo ever, except for a few PRs made by Roman. Honestly, to me, this is a practical red flag that this process is indeed complicated, especially if we want to use many different linters and grow the solution over time.

2) What is the exact cost in terms of developer experience?

Regarding the performance argument: I would like to know the exact cost we're about to pay here since it could make a big difference IMHO. Compilation has to be done very frequently in order to run tests. As I see it, a linter doesn't necessarily need to run as part of every compilation, but instead can be run before pushing the PR / when creating a commit / etc. This might make sense since linter errors are usually very easy to solve.

If this makes sense to you, then the performance gap has to be pretty big in order to really make a difference. Waiting 40 seconds instead of 20 to get all of the linter errors, for example, is a very low cost if not done frequently.

We need more answers, but currently, I think that I agree with Miguel and tend to favor a solution that costs less in terms of engineering time. As said above, the fact that no one added a linter to nogo is a big red flag for me. In the current situation we have a very fast and optimized linter that does almost nothing. Our solution should eventually be maintainable, usable and extendible in real-life.

Just my perspective & thoughts.

BR,

Itamar.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubevirt-dev/CAAx_ZVWLJ04h3X7%2BFFW81UGBq0xn57Mii-PFsgeZ3pvOS7T0bg%40mail.gmail.com.

[Roman Mohr]

On Mon, Jul 4, 2022 at 1:17 PM Miguel Duarte de Mora Barroso <mdba...@redhat.com> wrote:

On Thu, Jun 30, 2022 at 2:28 PM Daniel Hiller <dhi...@redhat.com> wrote:

On Thu, Jun 30, 2022 at 9:30 AM 'Roman Mohr' via kubevirt-dev <kubevi...@googlegroups.com> wrote:

On Wed, Jun 29, 2022 at 3:51 PM Dan Kenigsberg <dan...@redhat.com> wrote:

My broken git-diff version and Radim's job running it sneaked in before this discussion was concluded; I replaced git-diff with a one-liner bash as a quickfix. Any new job is expensive so we should have waited a bit longer before enabling it.

However, I'm happy that we have it enabled now. I failed to find a rich ecosystem of nogo linters to learn from. So lacking the one-liner, I don't believe anyone would have written a kubevirt nogo linter to block tests/utils from growing further by now.

If you search "bazel nogo" in google the first result is this link: https://github.com/bazelbuild/rules_go/blob/master/go/nogo.rst, it shows a complete linter implementation and how to register it.

Also note that I immediately shared this link in my first response on this email thread.

I understand that you really have a big issue with the file size and it is ok to make it shorter, but as you know this has lower priority from the maintainer side, so no one of us took this task.

If you really wish to have it, I would expect that you either spend the time on the research or convince someone to do it for you. If both things do not happen it may be ok to not have the check due to the obvious low priority then, instead of just moving forward and making the normal developer flow more difficult.

It looks like that you mostly spend a little bit of time on superficial improvements on the project without real intent to develop any features on kubevirt. This is completely fine and also welcome, however you may not

be in a position then to understand the pros of the current setup.

Developer experience (when not doing release engineering) has a pretty high standard at kubevirt, which makes release engineering tasks more complex and one has to reserve the time for that.

So, moving forward, wouldn't the best solution be to include all the wanted golangci-lint linters into nogo, as this should be easy to do, since they are implementing the analyzer interface and can be picked up by nogo? And also include the nogo filelength stuff from Roman. And as a last step removing the separate lint make target?

While it is now clear to me the developer experience provided by the nogo integrated linters is (quite) better, it is also clear that it takes more time to integrate them.

Great, first just understanding the current state before just blindly doing something is all I wanted. Thanks.

 

... and there are plenty of them.

I am not sure how many there are which we really want to add. I see a few different groups of linters:

1. 100% no false-positive linters which detect issues:

 * go vet
 * ineffassign

 * I saw a hand full of potentially interesting linters on the golangci-linter project

2. formatting linters

 * goimports

 * whitespace

3. tools which are known to report false-positives where one has to e.g. annotate code

 * gosec for example

kubevirt right now focuses on (1). I think that with gosec (3) we clearly saw that it is not very helpful to have to annotate a lot of code, which is then used in sub-functions of sub-functions, where simple refactorings make the statement that gosec reports are not harmful and were false positives invalid.

Then the code is annotated all over the place without meaning. There exist better ways (e.g. with bazel) to enforce certain imports and usages.

 For (2), instead of linting the code we reformat (like we don't have a goimports linter, but we directly use goimports to just format the code). I would not accept a linter job telling me that it wants me to remove a newline manually after an if statement, but I would accept a tool which would do that for me automatically.

 

The question - imho - is how long would it take a developer (or a series of developers) to introduce those linters one by one into nogo ? Is that worth the cost ? Honest question.

Take for instance Roman's work in bringing ineffassign into the project - it took from Jan 27, 2021 to Feb 13th 2021 to be integrated - yes, I realize we had to wait for a change

to be made to the linter's source code.

I could have wrapped it too immediately. I don't  think that adding additional linters is a task with time pressure. I don't want to trivialize it too much, I really think that we all have experiences with projects

where people frequently re-submitted because linters failed, which sums up over time (not on a single engineer but on the whole project). Which justifies in my opinion extra time.

I personally always try to have the overall effect for normal engineering work in the back of my mind. By saving time for everyone, I myself also win time on the long run which I can again invest in

improvements on testing and release engineering, keeping the feature- and bugfixing process fluid.

The current alternative, which works across the golang's ecosystem seamlessly, is available now - *but* provides a weaker dev experience.

The metric we should try to optimize is of course, code quality. But if the end result is the same, we reach the conundrum of engineering time + more code to maintain vs worse dev. experience. I tend to favor the solution that costs less in terms of engineering time, but that's just me.

What I tried to optimize is not the linter-implementation time lost, but the time which people may repeatedly lose when submitting PRs. Missing `make generate` invocations give you a pretty nice overview on what to expect with a dedicated linter job (except that it will happen much more often). 

I could not find a better way than the dedicated `make generate` (a compromise of having a single but long running entrypoint to regenerate everything vs. faster but more specific targets which devs have to know about on a lot of changes).

`make generate` significantly "wastes" time of engineers, but is luckily very often not needed to run, so I would consider it OK, given that we have to run some slow tools where we can't work around.

Now linting is much more common to fail if it is not executed always locally, leading to additional emails to `make generate` and test flakes already.

If linting jobs just fail a few times per week you probably already would have saved enough time to add another linter in a more complex way.

 

Anyway, I fully agree that if we're not making the golang-ci linters required, they have no place in the code, and should be removed. 

 

I'm actually even willing to try out integrating one of these linters into no-go once I can spare some time.

Which ones do you consider the most important ones?

Best regards,

Roman 

[Roman Mohr]

On Mon, Jul 4, 2022 at 3:26 PM Itamar Holder <iho...@redhat.com> wrote:

I think excellent questions were raised here that are important to discuss, and we haven't yet reached a solid conclusion.

To summarize, these are (in my eyes) the open questions that remain:

1) How easy/hard is it to add more linters into nogo?

On the one hand Roman said that it's "easy by just adding an _ import and running make deps-sync". On the other hand, as Miguel pointed out, the PR that introduced that was very long and complex and took time to merge. Even the PR for checking file length requires its fair amount of development work / review cycle, etc.

I am not sure why you think that the longfile PR is complex or requires a lot of reviews. In contrast to not easy extensible bash scripts it already covers the whole process around too long files, default case checking + exceptions, all that with roughly 40 lines of go code when leaving the boiler-plate go code aside which is hard to do wrong.

 

I also want to point out that the fact is that basically no one added any linter to nogo ever, except for a few PRs made by Roman.

Maybe there was a thread about adding linters somewhere which I missed, but I can't remember someone trying to directly integrate a linter in our code base (there was gosec, sonarcloud, ... which runs linters outside).
When I look at Edwards PR and mailing thread it looks to me like people were not even aware that linters already exist and thought that now is the time and that they can start on a green field.

 

Honestly, to me, this is a practical red flag that this process is indeed complicated, especially if we want to use many different linters and grow the solution over time.

 

I would be careful with just blindly adding a lot of linters. I think many of  them require extra discussion and may fall behind on what they seem to promise (like e.g. gosec).

2) What is the exact cost in terms of developer experience?

Regarding the performance argument: I would like to know the exact cost we're about to pay here since it could make a big difference IMHO. Compilation has to be done very frequently in order to run tests. As I see it, a linter doesn't necessarily need to run as part of every compilation, but instead can be run before pushing the PR / when creating a commit / etc. This might make sense since linter errors are usually very easy to solve.

If this makes sense to you, then the performance gap has to be pretty big in order to really make a difference. Waiting 40 seconds instead of 20 to get all of the linter errors, for example, is a very low cost if not done frequently.

Just to add a few numbers. I changed the linter target to finally lint the whole code-base.

Here I run the linter for the first time. It took 1m15s, consuming 10 cores to 100% on my machine with these specs:

 * Intel(R) Xeon(R) W-2135 CPU @ 3.70GHz 
 * 64GB ram 

```
$ time make lint
if [ $(wc -l < tests/utils.go) -gt 3565 ]; then echo >&2 "do not make tests/utils longer"; exit 1; fi
hack/dockerized "golangci-lint run --timeout 10m --verbose \
  pkg/... \
  tests/... \
  staging/... \
[...]

real 1m15.021s
user 0m0.579s
sys 0m0.515s
```

```
On subsequent runs where the cache was populated it took on my machine around 6-7 seconds.

$ time make lint
[...]
INFO File cache stats: 204 entries of total size 5.6MiB
INFO Memory: 35 samples, avg is 78.5MB, max is 125.4MB
INFO Execution took 3.343902672s                  
make: *** [Makefile:193: lint] Error 1

real 0m6.718s
user 0m0.545s
sys 0m0.492s
```

Would be interesting to see how long it takes on less advanced hardware.

In general `make` after the cache is populated takes 7 seconds for me, which I also consider unnecessary long, and there are ways to make it significantly faster.

In general, regarding to "developer cost", there are many aspects to it, and I think it is underestimated how much extra-work all kinds of small deviations from established paths add:

 * Dan added a bash script which does something (https://github.com/kubevirt/kubevirt/pull/7804)

 * Some want to work around the CI pipeline with github actions
 * Nahson wants to add a linter written in python ( https://github.com/kubevirt/kubevirt/pull/8043)
 * Edward wants to run the goimports linter in a different way than we auto-format them ( https://github.com/kubevirt/kubevirt/pull/8033#discussion_r912856101), meaning that from now on people

 * Trading developer time of all developers with making it simpler to add linters

We can very well decide to not use nogo, maybe even to not use bazel, but what is going on right now is a little bit scary in my opinion.

There seems to be an extreme pressure at some place invisible for the broader community to do "something" in this area, no matter what the costs are, without coordinating anything or researching much.

So as a maintainer who takes his work seriously I am really questioning if it is ok on what is going on.

Best regards,

Roman

To view this discussion on the web visit https://groups.google.com/d/msgid/kubevirt-dev/CALpNySx1zJJRDsnDfw%2B5rVLn9Js3tzjqs2gzqjyaZ6vXOe2kZg%40mail.gmail.com.

[Daniel Hiller]

OK, I'll try to go back a step here.

TBH I think that the whole flow of a separate CI lint lane is not a good experience, because:

1) it catches things too late

2) it is a separate step

3) it is another tool

Regarding 1) and 2)

The lint lane kicks in when a commit to a PR is pushed. From my experience people for many reasons do not or do forget to run a linter that is not integrated into the local build process. This produces a high probability of linting errors getting caught too late, while at the same time wasting CI resources that would not have been needed if the linter was run earlier (i.e. even before the local commit was done). IIUC nogo is integrated into the build for exactly that reason. If people see fit that golangci-lint should be required, it should be integrated into building the project. This way people unfamiliar to the project will not wonder about what all that linting stuff is about and still obey the general rules. Please consider that not all developers might be completely familiar with why linting is a good practice in the development process.

Regarding 3)

introducing another tool costs developer resources learning and managing it. I don't say that this is unwanted in our project, but I think that the project is already complex and adding to complexity increases the burden for developers. I don't say that using nogo is easy, but when looking at tools that do the same thing, I tend to pick the most suitable of those and throw away the rest.

So IMHO a good linter should have the following properties

1) be fast

2) be easy to use

3) be easy to integrate

3) be easy to extend

If golangci-lint has all these and if it is easier to use than nogo, then let's integrate it into the project's build process. OTOH IIUC nogo runs all the analyzers in parallel and caches all results, so it should be super fast. IDK whether golangci-lint does the same, so if it does, please disregard.

And regarding 4) from above then we could adapt Roman's linter checking for file length and integrate it into the golangci-lint and get rid of the lint-lane, no? 

On Mon, Jul 4, 2022 at 3:26 PM Itamar Holder <iho...@redhat.com> wrote:

--

-- 

Best,

Daniel

[Edward Haas]

On Tue, Jul 5, 2022 at 11:52 AM Daniel Hiller <dhi...@redhat.com> wrote:

OK, I'll try to go back a step here.

TBH I think that the whole flow of a separate CI lint lane is not a good experience, because:

1) it catches things too late

2) it is a separate step

3) it is another tool

Regarding 1) and 2)

The lint lane kicks in when a commit to a PR is pushed. From my experience people for many reasons do not or do forget to run a linter that is not integrated into the local build process. This produces a high probability of linting errors getting caught too late, while at the same time wasting CI resources that would not have been needed if the linter was run earlier (i.e. even before the local commit was done). IIUC nogo is integrated into the build for exactly that reason. If people see fit that golangci-lint should be required, it should be integrated into building the project. This way people unfamiliar to the project will not wonder about what all that linting stuff is about and still obey the general rules. Please consider that not all developers might be completely familiar with why linting is a good practice in the development process.

The same reasoning presented here is true for unit tests as well.

As a contributor, I would like to get service from the CI, no to be forced to calculate my moves and standards based on its load.

If the CI has limitations, I expect it to manage the workloads differently and to fit the usage needs (not the other way around).

The integration of nogo into the build is one aspect out of many others.

If I remember right, 3 points have been raised regarding its pros:

- Runs as part of the build.

- Runs in parallel (fast).

- Uses Bazel cache.

Running part of the build is easily solvable by adding the lint target under the default one.

If this solves some part of a user experience, I doubt it, but it will solve the point raised about it.

The golangci-lint tool runner is also running in parallel, so that point is checked as well.

The part about the bazel cache is obviously not available. While it uses a cache when triggered multiple times, this is not

serving the advantages bazel has when sharing cache between PR/s.

I find this to be its only weak point. And this weak point, IMO, is not that dramatic at this stage.

Regarding 3)

introducing another tool costs developer resources learning and managing it. I don't say that this is unwanted in our project, but I think that the project is already complex and adding to complexity increases the burden for developers. I don't say that using nogo is easy, but when looking at tools that do the same thing, I tend to pick the most suitable of those and throw away the rest.

So IMHO a good linter should have the following properties

1) be fast

2) be easy to use

3) be easy to integrate

3) be easy to extend

If golangci-lint has all these and if it is easier to use than nogo, then let's integrate it into the project's build process. OTOH IIUC nogo runs all the analyzers in parallel and caches all results, so it should be super fast. IDK whether golangci-lint does the same, so if it does, please disregard.

I think we also need to account for community adoption and the maintenance cost.

Using the same linter across the organization, it has its own awards. This is already happening.

Taking advantage of community investment in linters, with the experience they bring, makes our project better, it makes us follow good practices

others found useful and found the energy to solve.

While acknowledging the advantages that nogo brings, I cannot ignore the lint runner that is so common among the Go community.

It has far better documentation, far many users and linters and it can be used everywhere, regardless of having Bazel or not.

And regarding 4) from above then we could adapt Roman's linter checking for file length and integrate it into the golangci-lint and get rid of the lint-lane, no? 

I found the golangci-lint useful in cleaning up code, from both bugs and opinionated patterns.

Using the accumulated experience in using the linter in other projects, its documentation and its larger community where answers can be found,

I would prefer to continue using it over investing in an in-house integration.

Using the file-length linter is an excellent example of a valuable piece of linter that will be limited to kubevirt/kubevirt.

We will not be able to use it in our other projects, nor to be nice and contribute it to the Go community.

It will also be left up to us to maintain.

So the functionality is much better, but the cost for the long run is much higher (one liner script vs a Go program).

I would prefer to have the good from both worlds. I also do not think the file length is the main concern here.

In the end, we want to increase the code quality to reduce cluttering when reading code, and help reviewers

and contributors to understand what the code does.

One way to do it (and there are a lot of things beyond it), is to place an opinionated linter in place to set a standard for all to follow.

I think it is worth the extra cost to introduce it and to consider any solution that addresses this target.

[Daniel Hiller]

On Tue, Jul 5, 2022 at 11:40 AM Edward Haas <edw...@redhat.com> wrote:

On Tue, Jul 5, 2022 at 11:52 AM Daniel Hiller <dhi...@redhat.com> wrote:

OK, I'll try to go back a step here.

TBH I think that the whole flow of a separate CI lint lane is not a good experience, because:

1) it catches things too late

2) it is a separate step

3) it is another tool

Regarding 1) and 2)

The lint lane kicks in when a commit to a PR is pushed. From my experience people for many reasons do not or do forget to run a linter that is not integrated into the local build process. This produces a high probability of linting errors getting caught too late, while at the same time wasting CI resources that would not have been needed if the linter was run earlier (i.e. even before the local commit was done). IIUC nogo is integrated into the build for exactly that reason. If people see fit that golangci-lint should be required, it should be integrated into building the project. This way people unfamiliar to the project will not wonder about what all that linting stuff is about and still obey the general rules. Please consider that not all developers might be completely familiar with why linting is a good practice in the development process.

The same reasoning presented here is true for unit tests as well.

I disagree. Just running `make bazel-test` instead of `make bazel-build` solves this. So the integration is there, while it's not for `make lint`. I assumed that the latter might be intended, which implied that it runs mostly on CI, which I disagree with, as stated above.

 

As a contributor, I would like to get service from the CI, no to be forced to calculate my moves and standards based on its load.

If the CI has limitations, I expect it to manage the workloads differently and to fit the usage needs (not the other way around).

Here I am not saying that we should always be aware of CI, I just wanted to make the implication clear. Yes, service from CI should be expected, but there are no unlimited CI resources, which is why one cannot expect this. CI is there to catch things the developer did miss (and not because out of lazyness, that is NOT what CI is for), and if a system can catch things locally before going into a PR, then it should be in place.

 

The integration of nogo into the build is one aspect out of many others.

If I remember right, 3 points have been raised regarding its pros:

- Runs as part of the build.

- Runs in parallel (fast).

- Uses Bazel cache.

Running part of the build is easily solvable by adding the lint target under the default one.

If this solves some part of a user experience, I doubt it, but it will solve the point raised about it.

The golangci-lint tool runner is also running in parallel, so that point is checked as well.

The part about the bazel cache is obviously not available. While it uses a cache when triggered multiple times, this is not

serving the advantages bazel has when sharing cache between PR/s.

I find this to be its only weak point. And this weak point, IMO, is not that dramatic at this stage.

Regarding 3)

introducing another tool costs developer resources learning and managing it. I don't say that this is unwanted in our project, but I think that the project is already complex and adding to complexity increases the burden for developers. I don't say that using nogo is easy, but when looking at tools that do the same thing, I tend to pick the most suitable of those and throw away the rest.

So IMHO a good linter should have the following properties

1) be fast

2) be easy to use

3) be easy to integrate

3) be easy to extend

If golangci-lint has all these and if it is easier to use than nogo, then let's integrate it into the project's build process. OTOH IIUC nogo runs all the analyzers in parallel and caches all results, so it should be super fast. IDK whether golangci-lint does the same, so if it does, please disregard.

I think we also need to account for community adoption and the maintenance cost.

Using the same linter across the organization, it has its own awards. This is already happening.

Taking advantage of community investment in linters, with the experience they bring, makes our project better, it makes us follow good practices

others found useful and found the energy to solve.

Sure, but this is a different stance, as it brings in another tool to an existing project. In a greenfield, I'd agree that this is a good thing (bringing in a linter where nothing is present already). We already have seen how problematic this has been with gosec.

 

While acknowledging the advantages that nogo brings, I cannot ignore the lint runner that is so common among the Go community.

It has far better documentation, far many users and linters and it can be used everywhere, regardless of having Bazel or not.

Sure, again, for new projects, I completely agree. But there's other things that are less invasive which we can look at, i.e. Sonarcloud: https://sonarcloud.io/project/overview?id=kubevirt_kubevirt

And regarding 4) from above then we could adapt Roman's linter checking for file length and integrate it into the golangci-lint and get rid of the lint-lane, no? 

I found the golangci-lint useful in cleaning up code, from both bugs and opinionated patterns.

Using the accumulated experience in using the linter in other projects, its documentation and its larger community where answers can be found,

I would prefer to continue using it over investing in an in-house integration.

Using the file-length linter is an excellent example of a valuable piece of linter that will be limited to kubevirt/kubevirt.

We will not be able to use it in our other projects, nor to be nice and contribute it to the Go community.

It will also be left up to us to maintain.

So the functionality is much better, but the cost for the long run is much higher (one liner script vs a Go program).

I would prefer to have the good from both worlds. I also do not think the file length is the main concern here.

In the end, we want to increase the code quality to reduce cluttering when reading code, and help reviewers

and contributors to understand what the code does.

One way to do it (and there are a lot of things beyond it), is to place an opinionated linter in place to set a standard for all to follow.

I think it is worth the extra cost to introduce it and to consider any solution that addresses this target.

To be clear: I find the initiative to raise code quality to be a good thing, while at the same time I disagree with how it's done.

"THE BOY SCOUTS HAVE A RULE: “Always leave the campground cleaner than you found it.” " (Uncle Bob)

https://www.oreilly.com/library/view/97-things-every/9780596809515/ch08.html#:~:text=Robert%20C.,the%20next%20group%20of%20campers.

From my interpretation of this rule, what should happen should be this:

* you want to implement a feature

* you go to the place where you need to change the code

* you clean up

* you do your changes to implement a feature

* you run all sanity checks locally

* you post your PR

While it's fine to just clean up the code, I think the most value is created when adding new features. In the end the clean up of code should be an integral part of how we add new features.

Side note: the main metric I generally look at how cyclomatic complexity values are. A high value there is a great indicator where cleanup of code might be required. See here for a list: https://sonarcloud.io/component_measures?metric=complexity&view=list&id=kubevirt_kubevirt

I'd recommend adding a local plugin for your IDE that calculates this, which should make the code already much more readable.

--

-- 

Best,

Daniel

[Miguel Duarte de Mora Barroso]

errcheck, definitely. All others that check dead code / unused variables / struct members could also be interesting.

[Roman Mohr]

On Tue, Jul 5, 2022 at 11:40 AM Edward Haas <edw...@redhat.com> wrote:

On Tue, Jul 5, 2022 at 11:52 AM Daniel Hiller <dhi...@redhat.com> wrote:

OK, I'll try to go back a step here.

TBH I think that the whole flow of a separate CI lint lane is not a good experience, because:

1) it catches things too late

2) it is a separate step

3) it is another tool

Regarding 1) and 2)

The lint lane kicks in when a commit to a PR is pushed. From my experience people for many reasons do not or do forget to run a linter that is not integrated into the local build process. This produces a high probability of linting errors getting caught too late, while at the same time wasting CI resources that would not have been needed if the linter was run earlier (i.e. even before the local commit was done). IIUC nogo is integrated into the build for exactly that reason. If people see fit that golangci-lint should be required, it should be integrated into building the project. This way people unfamiliar to the project will not wonder about what all that linting stuff is about and still obey the general rules. Please consider that not all developers might be completely familiar with why linting is a good practice in the development process.

The same reasoning presented here is true for unit tests as well.

As a contributor, I would like to get service from the CI, no to be forced to calculate my moves and standards based on its load.

If the CI has limitations, I expect it to manage the workloads differently and to fit the usage needs (not the other way around).

This is a strange way of putting how things work at kubevirt. That kind of feedback you get in a considerable short time, especially considering the CI budget and capacity constraint is one of the highlights of kubevirt.

Especially that I get almost 100% the same feedback as on CI also locally.

You expect from the upstream project to immediately accept steps-backward if it fits your vision, but are not willing to accept any compromise on other parts (and here the compromise is to consider that you only get excellent feedback if you don't make one-line-PRs in an extremely high frequency).

I also wish capacity would be less of a problem, but the current structure safed on my code changes alone dozens, if not hundred(s) of bugs. Bugs which would have gone to various QE departments of adopters, to upstream users and paying customers of adopters.

There is a considerable amount of time spent to continue providing good feedback, while keeping the annoyances for developers on a minimal level. I almost feel blind when contributing to many other projects upstream. A lot of the improvements were by the way blocked because you had "concerns" on all kinds of things and I tried to stay friendly and patient.

With your constant ask for just following an ideal PR idea without considering such pros and cons, people think they are doing a good thing when doing something like https://github.com/kubevirt/kubevirt/pulls?q=is%3Apr+author%3Aiholder-redhat+is%3Aclosed+HaveLen (and I am not blaming the author of doing this). Or like you did at https://github.com/kubevirt/kubevirt/pulls?q=is%3Apr+is%3Aclosed+author%3AEdDev+%22pkg%2C+cleanup%22.

This costs a lot of time for all involved and almost feels like you are deliberately doing this to prove your point. Especially when doing this at a point where CI is not super healthy. I also know that you were always aware of my recommendations at this point in the project. This leaves kind of a weird picture.

With the linters, in contrast to some CI resource restrictions, we have a clear path with good developer experience which should at least be explored.

We can still argue at any time if bazel and all the CI work is worth it, but we can not just act like nothing exists and everything is done fresh (where adding something kind-of-working is often better than not having it).

Best regards,

Roman

[Edward Haas]

On Tue, Jul 5, 2022 at 1:41 PM Daniel Hiller <dhi...@redhat.com> wrote:

On Tue, Jul 5, 2022 at 11:40 AM Edward Haas <edw...@redhat.com> wrote:

On Tue, Jul 5, 2022 at 11:52 AM Daniel Hiller <dhi...@redhat.com> wrote:

OK, I'll try to go back a step here.

TBH I think that the whole flow of a separate CI lint lane is not a good experience, because:

1) it catches things too late

2) it is a separate step

3) it is another tool

Regarding 1) and 2)

The lint lane kicks in when a commit to a PR is pushed. From my experience people for many reasons do not or do forget to run a linter that is not integrated into the local build process. This produces a high probability of linting errors getting caught too late, while at the same time wasting CI resources that would not have been needed if the linter was run earlier (i.e. even before the local commit was done). IIUC nogo is integrated into the build for exactly that reason. If people see fit that golangci-lint should be required, it should be integrated into building the project. This way people unfamiliar to the project will not wonder about what all that linting stuff is about and still obey the general rules. Please consider that not all developers might be completely familiar with why linting is a good practice in the development process.

The same reasoning presented here is true for unit tests as well.

I disagree. Just running `make bazel-test` instead of `make bazel-build` solves this. So the integration is there, while it's not for `make lint`. I assumed that the latter might be intended, which implied that it runs mostly on CI, which I disagree with, as stated above.

This is exactly how we can solve this point with the linter when it is not embedded in the build.

You add it to an existing target as a dependency.

 

As a contributor, I would like to get service from the CI, no to be forced to calculate my moves and standards based on its load.

If the CI has limitations, I expect it to manage the workloads differently and to fit the usage needs (not the other way around).

Here I am not saying that we should always be aware of CI, I just wanted to make the implication clear. Yes, service from CI should be expected, but there are no unlimited CI resources, which is why one cannot expect this. CI is there to catch things the developer did miss (and not because out of lazyness, that is NOT what CI is for), and if a system can catch things locally before going into a PR, then it should be in place.

While I am accustomed to run locally common targets, I recognize that there are reasons

for contributors not running something locally. Moreover, running locally is not an assurance it will pass CI (which runs with a different config in most cases).

However one decides to look at this, I think that assuming anything about the usage is not safe and therefore undesirable.

If the development flow can enforce a behavior, then assumptions of usage are reasonable.

I do not think we are in that position at the moment.

 

The integration of nogo into the build is one aspect out of many others.

If I remember right, 3 points have been raised regarding its pros:

- Runs as part of the build.

- Runs in parallel (fast).

- Uses Bazel cache.

Running part of the build is easily solvable by adding the lint target under the default one.

If this solves some part of a user experience, I doubt it, but it will solve the point raised about it.

The golangci-lint tool runner is also running in parallel, so that point is checked as well.

The part about the bazel cache is obviously not available. While it uses a cache when triggered multiple times, this is not

serving the advantages bazel has when sharing cache between PR/s.

I find this to be its only weak point. And this weak point, IMO, is not that dramatic at this stage.

Regarding 3)

introducing another tool costs developer resources learning and managing it. I don't say that this is unwanted in our project, but I think that the project is already complex and adding to complexity increases the burden for developers. I don't say that using nogo is easy, but when looking at tools that do the same thing, I tend to pick the most suitable of those and throw away the rest.

So IMHO a good linter should have the following properties

1) be fast

2) be easy to use

3) be easy to integrate

3) be easy to extend

If golangci-lint has all these and if it is easier to use than nogo, then let's integrate it into the project's build process. OTOH IIUC nogo runs all the analyzers in parallel and caches all results, so it should be super fast. IDK whether golangci-lint does the same, so if it does, please disregard.

I think we also need to account for community adoption and the maintenance cost.

Using the same linter across the organization, it has its own awards. This is already happening.

Taking advantage of community investment in linters, with the experience they bring, makes our project better, it makes us follow good practices

others found useful and found the energy to solve.

Sure, but this is a different stance, as it brings in another tool to an existing project. In a greenfield, I'd agree that this is a good thing (bringing in a linter where nothing is present already). We already have seen how problematic this has been with gosec.

I want to hope that things can change to make things better.

Adjusting to new patterns, new tools and new ideas is something I want to see happening in both existing and new projects.

 

While acknowledging the advantages that nogo brings, I cannot ignore the lint runner that is so common among the Go community.

It has far better documentation, far many users and linters and it can be used everywhere, regardless of having Bazel or not.

Sure, again, for new projects, I completely agree. But there's other things that are less invasive which we can look at, i.e. Sonarcloud: https://sonarcloud.io/project/overview?id=kubevirt_kubevirt

I recognize that there is more than one solution.

The current one with gloangci-lint was proposed and pushed to execution with a clear plan on how to go forward.

We are left to enforce it so no one will override the code quality in areas that have been declared "green".

My ability to contribute something that I can stand behind was golangci-lint per my experience with it.

Considering this is also the tool used for other projects in the organization and beyond, I feel it is a good choice.

Said that, I am not against other options if it can do the same, expand and be controlled (choose linters, configure their behavior).

The rule is great and I see many of my colleges apply it.

I see no contradiction between such a rule and the tooling made to assist follow it.

The tools help detect common patterns to avoid and patterns to preserve to increase code standards.

I see the tool as a static assistant to make clean code and to make sure it is preserved as such in the future.

I would prefer to keep my development setup out of this discussion, just to remove assumptions.

There is individual freedom to use local tooling (including IDE/s and its plugins) as one sees fit.

At the same time, there is a project standard that is to be kept and enforced.

[Edward Haas]

On Tue, Jul 5, 2022 at 2:36 PM Roman Mohr <rm...@google.com> wrote:

On Tue, Jul 5, 2022 at 11:40 AM Edward Haas <edw...@redhat.com> wrote:

On Tue, Jul 5, 2022 at 11:52 AM Daniel Hiller <dhi...@redhat.com> wrote:

OK, I'll try to go back a step here.

TBH I think that the whole flow of a separate CI lint lane is not a good experience, because:

1) it catches things too late

2) it is a separate step

3) it is another tool

Regarding 1) and 2)

The lint lane kicks in when a commit to a PR is pushed. From my experience people for many reasons do not or do forget to run a linter that is not integrated into the local build process. This produces a high probability of linting errors getting caught too late, while at the same time wasting CI resources that would not have been needed if the linter was run earlier (i.e. even before the local commit was done). IIUC nogo is integrated into the build for exactly that reason. If people see fit that golangci-lint should be required, it should be integrated into building the project. This way people unfamiliar to the project will not wonder about what all that linting stuff is about and still obey the general rules. Please consider that not all developers might be completely familiar with why linting is a good practice in the development process.

The same reasoning presented here is true for unit tests as well.

As a contributor, I would like to get service from the CI, no to be forced to calculate my moves and standards based on its load.

If the CI has limitations, I expect it to manage the workloads differently and to fit the usage needs (not the other way around).

This is a strange way of putting how things work at kubevirt. That kind of feedback you get in a considerable short time, especially considering the CI budget and capacity constraint is one of the highlights of kubevirt.

Especially that I get almost 100% the same feedback as on CI also locally.

We have very different experiences then.

I am not getting the same results locally, mainly because of the CI infrastructure setup.

You expect from the upstream project to immediately accept steps-backward if it fits your vision, but are not willing to accept any compromise on other parts (and here the compromise is to consider that you only get excellent feedback if you don't make one-line-PRs in an extremely high frequency).

Unfortunately I do compromise, and the cost is very high for me.

The cognitive energy required from me to perform a review on large PR/s is significant.

My ability to detect bugs in others' work and even in mine is severely degraded when the PR is big.

But even if I ignore all these, the option to assume anything about the usage is not there.

Contributors will eventually choose the most comfortable path and not the one the CI works best with.

If the suggestion is to control the usage, proper means should be in place to enforce it. Assumptions is not an option IMO.

I also wish capacity would be less of a problem, but the current structure safed on my code changes alone dozens, if not hundred(s) of bugs. Bugs which would have gone to various QE departments of adopters, to upstream users and paying customers of adopters.

There is a considerable amount of time spent to continue providing good feedback, while keeping the annoyances for developers on a minimal level. I almost feel blind when contributing to many other projects upstream.

 

I value the feedback from the tooling we have.

I am not seeking to remove it, I am seeking to improve it so it will be beneficial to the needs we have.

A lot of the improvements were by the way blocked because you had "concerns" on all kinds of things and I tried to stay friendly and patient.

With your constant ask for just following an ideal PR idea without considering such pros and cons, people think they are doing a good thing when doing something like https://github.com/kubevirt/kubevirt/pulls?q=is%3Apr+author%3Aiholder-redhat+is%3Aclosed+HaveLen (and I am not blaming the author of doing this). Or like you did at https://github.com/kubevirt/kubevirt/pulls?q=is%3Apr+is%3Aclosed+author%3AEdDev+%22pkg%2C+cleanup%22.

This costs a lot of time for all involved and almost feels like you are deliberately doing this to prove your point. Especially when doing this at a point where CI is not super healthy. I also know that you were always aware of my recommendations at this point in the project. This leaves kind of a weird picture.

There is an attempt to improve test failures readability (also in logs output) and push for caring about code quality.

I do not believe anyone that contributed code to the project is looking to hurt it, I think most want to see it being improved and raise attention that

code quality matters.

With the linters, in contrast to some CI resource restrictions, we have a clear path with good developer experience which should at least be explored.

We can still argue at any time if bazel and all the CI work is worth it, but we can not just act like nothing exists and everything is done fresh (where adding something kind-of-working is often better than not having it).

I am good with negotiating about other toolings to linter.

In fact my impression is that this thread does exactly that.

It seemed to me that there have been only a very few maintainers (1?) who knew that nogo exists.

And we did evaluate it in comparison to the golangci-lint once it was exposed. With this thread we still do.

At this point of time, I am in favor of both being pushed forward and learning the cost of each so we can

re-evaluate it once a proper comparison can be made.

I know of a few contributors that have experience with golangci-lint from other projects, I find it easy and straightforward

to use it anywhere. I have no intentions to block contributions to nogo, unfortunately I do not see myself investing in it at the moment.

[Daniel Hiller]

On Tue, Jul 5, 2022 at 1:41 PM Edward Haas <edw...@redhat.com> wrote:

On Tue, Jul 5, 2022 at 1:41 PM Daniel Hiller <dhi...@redhat.com> wrote:

On Tue, Jul 5, 2022 at 11:40 AM Edward Haas <edw...@redhat.com> wrote:

On Tue, Jul 5, 2022 at 11:52 AM Daniel Hiller <dhi...@redhat.com> wrote:

OK, I'll try to go back a step here.

TBH I think that the whole flow of a separate CI lint lane is not a good experience, because:

1) it catches things too late

2) it is a separate step

3) it is another tool

Regarding 1) and 2)

The lint lane kicks in when a commit to a PR is pushed. From my experience people for many reasons do not or do forget to run a linter that is not integrated into the local build process. This produces a high probability of linting errors getting caught too late, while at the same time wasting CI resources that would not have been needed if the linter was run earlier (i.e. even before the local commit was done). IIUC nogo is integrated into the build for exactly that reason. If people see fit that golangci-lint should be required, it should be integrated into building the project. This way people unfamiliar to the project will not wonder about what all that linting stuff is about and still obey the general rules. Please consider that not all developers might be completely familiar with why linting is a good practice in the development process.

The same reasoning presented here is true for unit tests as well.

I disagree. Just running `make bazel-test` instead of `make bazel-build` solves this. So the integration is there, while it's not for `make lint`. I assumed that the latter might be intended, which implied that it runs mostly on CI, which I disagree with, as stated above.

This is exactly how we can solve this point with the linter when it is not embedded in the build.

You add it to an existing target as a dependency.

And as said before, nogo is already integrated into the bazel build process. The solution you are suggesting is not the same, as IIUC this would be added as a make dependency, while an integration on the bazel level would be favorable.

 

As a contributor, I would like to get service from the CI, no to be forced to calculate my moves and standards based on its load.

If the CI has limitations, I expect it to manage the workloads differently and to fit the usage needs (not the other way around).

Here I am not saying that we should always be aware of CI, I just wanted to make the implication clear. Yes, service from CI should be expected, but there are no unlimited CI resources, which is why one cannot expect this. CI is there to catch things the developer did miss (and not because out of lazyness, that is NOT what CI is for), and if a system can catch things locally before going into a PR, then it should be in place.

While I am accustomed to run locally common targets, I recognize that there are reasons

for contributors not running something locally.

What reasons exactly do you refer to here?

 

Moreover, running locally is not an assurance it will pass CI (which runs with a different config in most cases).

No, it's not, mostly because tests are flaky, but that is something we are trying to solve. BTW what different configs do you mean? The bazel cache? The image cache? What else am I overlooking here?

 

However one decides to look at this, I think that assuming anything about the usage is not safe and therefore undesirable.

I can't follow that reasoning? Can you elaborate? We are talking about developers here using tools, where each one has a certain usage, and if you aren't using it that way, you are holding it wrong. 

 

If the development flow can enforce a behavior, then assumptions of usage are reasonable.

I do not think we are in that position at the moment.

Please elaborate on this one.

I agree when the effort is worth the benefit. 

 

While acknowledging the advantages that nogo brings, I cannot ignore the lint runner that is so common among the Go community.

It has far better documentation, far many users and linters and it can be used everywhere, regardless of having Bazel or not.

Sure, again, for new projects, I completely agree. But there's other things that are less invasive which we can look at, i.e. Sonarcloud: https://sonarcloud.io/project/overview?id=kubevirt_kubevirt

I recognize that there is more than one solution.

The current one with gloangci-lint was proposed and pushed to execution with a clear plan on how to go forward.

We are left to enforce it so no one will override the code quality in areas that have been declared "green".

My ability to contribute something that I can stand behind was golangci-lint per my experience with it.

Considering this is also the tool used for other projects in the organization and beyond, I feel it is a good choice.

Said that, I am not against other options if it can do the same, expand and be controlled (choose linters, configure their behavior).

 

To be clear, I think that golanglint-ci looks like an excellent choice, all I am saying is that it should be integrated better.

I wasn't saying that one must install a special tool, my point above was that in general there is more value in adding a feature than in cleaning up on it's own.

--

-- 

Best,

Daniel

[Roman Mohr]

On Tue, Jul 5, 2022 at 1:41 PM Edward Haas <edw...@redhat.com> wrote:

On Tue, Jul 5, 2022 at 1:41 PM Daniel Hiller <dhi...@redhat.com> wrote:

On Tue, Jul 5, 2022 at 11:40 AM Edward Haas <edw...@redhat.com> wrote:

On Tue, Jul 5, 2022 at 11:52 AM Daniel Hiller <dhi...@redhat.com> wrote:

OK, I'll try to go back a step here.

TBH I think that the whole flow of a separate CI lint lane is not a good experience, because:

1) it catches things too late

2) it is a separate step

3) it is another tool

Regarding 1) and 2)

The lint lane kicks in when a commit to a PR is pushed. From my experience people for many reasons do not or do forget to run a linter that is not integrated into the local build process. This produces a high probability of linting errors getting caught too late, while at the same time wasting CI resources that would not have been needed if the linter was run earlier (i.e. even before the local commit was done). IIUC nogo is integrated into the build for exactly that reason. If people see fit that golangci-lint should be required, it should be integrated into building the project. This way people unfamiliar to the project will not wonder about what all that linting stuff is about and still obey the general rules. Please consider that not all developers might be completely familiar with why linting is a good practice in the development process.

The same reasoning presented here is true for unit tests as well.

I disagree. Just running `make bazel-test` instead of `make bazel-build` solves this. So the integration is there, while it's not for `make lint`. I assumed that the latter might be intended, which implied that it runs mostly on CI, which I disagree with, as stated above.

This is exactly how we can solve this point with the linter when it is not embedded in the build.

You add it to an existing target as a dependency.

 

As a contributor, I would like to get service from the CI, no to be forced to calculate my moves and standards based on its load.

If the CI has limitations, I expect it to manage the workloads differently and to fit the usage needs (not the other way around).

Here I am not saying that we should always be aware of CI, I just wanted to make the implication clear. Yes, service from CI should be expected, but there are no unlimited CI resources, which is why one cannot expect this. CI is there to catch things the developer did miss (and not because out of lazyness, that is NOT what CI is for), and if a system can catch things locally before going into a PR, then it should be in place.

While I am accustomed to run locally common targets, I recognize that there are reasons

for contributors not running something locally. Moreover, running locally is not an assurance it will pass CI (which runs with a different config in most cases).

However one decides to look at this, I think that assuming anything about the usage is not safe and therefore undesirable.

If the development flow can enforce a behavior, then assumptions of usage are reasonable.

I do not think we are in that position at the moment.

Feel free to continue letting upstream CI do the validation. CI will do its work. Just keep in mind CI capacity. There are other people on the project.

So far mostly you are ignoring soft stops forcing us to even think about hard stops, which we generally want to avoid as much as possible.

Don't expect us to e.g. test less to use less CI capacity so that you can create more PRs. If you want to let upstream CI do all the work (I mean it anyway has to, but even including iterating on your code changes), that is a very very inefficient pattern where you

are sacrificing quite a bit of possibilities to move faster. No one forces you to move fast. That is up to you.

I think the tools and flows are already way ahead of what you want to establish.

I can understand that in "your ideal developer flow" they may not be too much of a problem, but I don't understand why we don't force developers

to code the most efficient way, we don't have to care about the most efficient way. The solutions supporting the more efficient development flows are not in the way for upstream CI to do the validation, but the other way round does not work.

You get a lot of freedom on kubevirt. We really only limit wherever necessary, so please be mindful of that freedom.

Best regards,

Roman

[Edward Haas]

On Tue, Jul 5, 2022 at 3:24 PM Daniel Hiller <dhi...@redhat.com> wrote:

On Tue, Jul 5, 2022 at 1:41 PM Edward Haas <edw...@redhat.com> wrote:

On Tue, Jul 5, 2022 at 1:41 PM Daniel Hiller <dhi...@redhat.com> wrote:

On Tue, Jul 5, 2022 at 11:40 AM Edward Haas <edw...@redhat.com> wrote:

On Tue, Jul 5, 2022 at 11:52 AM Daniel Hiller <dhi...@redhat.com> wrote:

OK, I'll try to go back a step here.

TBH I think that the whole flow of a separate CI lint lane is not a good experience, because:

1) it catches things too late

2) it is a separate step

3) it is another tool

Regarding 1) and 2)

The lint lane kicks in when a commit to a PR is pushed. From my experience people for many reasons do not or do forget to run a linter that is not integrated into the local build process. This produces a high probability of linting errors getting caught too late, while at the same time wasting CI resources that would not have been needed if the linter was run earlier (i.e. even before the local commit was done). IIUC nogo is integrated into the build for exactly that reason. If people see fit that golangci-lint should be required, it should be integrated into building the project. This way people unfamiliar to the project will not wonder about what all that linting stuff is about and still obey the general rules. Please consider that not all developers might be completely familiar with why linting is a good practice in the development process.

The same reasoning presented here is true for unit tests as well.

I disagree. Just running `make bazel-test` instead of `make bazel-build` solves this. So the integration is there, while it's not for `make lint`. I assumed that the latter might be intended, which implied that it runs mostly on CI, which I disagree with, as stated above.

This is exactly how we can solve this point with the linter when it is not embedded in the build.

You add it to an existing target as a dependency.

And as said before, nogo is already integrated into the bazel build process. The solution you are suggesting is not the same, as IIUC this would be added as a make dependency, while an integration on the bazel level would be favorable.

I find the cache part the meaningful advantage, not the fact that it is already wired into bazel build flow.

If we look at the usage/call, adding it to a make target has the same result.

 

As a contributor, I would like to get service from the CI, no to be forced to calculate my moves and standards based on its load.

If the CI has limitations, I expect it to manage the workloads differently and to fit the usage needs (not the other way around).

Here I am not saying that we should always be aware of CI, I just wanted to make the implication clear. Yes, service from CI should be expected, but there are no unlimited CI resources, which is why one cannot expect this. CI is there to catch things the developer did miss (and not because out of lazyness, that is NOT what CI is for), and if a system can catch things locally before going into a PR, then it should be in place.

While I am accustomed to run locally common targets, I recognize that there are reasons

for contributors not running something locally.

What reasons exactly do you refer to here?

There are the non-intentional ones, e.g.:
- Forgot to run it.
- Run it on the wrong branch/folder.
- Have other files locally that have not been committed,

- Have files removed that have not been committed.

And there are intentional ones, e.g.:
- Unable to run it locally temporarily due to some setup/OS problem (e.g. docker crashes).

- Some devs use remote machines to run their targets, these may be inaccessible due to outage, maintenance or other disruptions.

- Some will like to contribute to less powerful computers (e.g. students) which cannot run some targets.

 

Moreover, running locally is not an assurance it will pass CI (which runs with a different config in most cases).

No, it's not, mostly because tests are flaky, but that is something we are trying to solve. BTW what different configs do you mean? The bazel cache? The image cache? What else am I overlooking here?

Different CPU architecture, core count, core speed, kernel version, disk speed, disk speed, etc...

Having leftover cache locally can also affect it, including non committed work that can be forgotten.

 

However one decides to look at this, I think that assuming anything about the usage is not safe and therefore undesirable.

I can't follow that reasoning? Can you elaborate? We are talking about developers here using tools, where each one has a certain usage, and if you aren't using it that way, you are holding it wrong. 

You may consider the developer using it wrong or not using it at all, but a CI system cannot assume anything about the usage.

It is up to the CI system to assure that what it examines is correct, anything else, including local runs are not trusted.

In other words, what counts is what the CI says, that is the only thing you can trust.

 

If the development flow can enforce a behavior, then assumptions of usage are reasonable.

I do not think we are in that position at the moment.

Please elaborate on this one.

If you can force the developer to perform a task and only then to push to the CI, only then you can trust it was done.

I do not know how you can do that really.

Expecting and hoping contributors followed the project recommendation before pushing is not something you can assume IMO.

There is always a balance between new features and keeping the codebase clean.

There is a point where it is hard to add new features and therefore you have no choice but to clean.

[Nahshon Unna Tsameret]

Hi.

I'm working on a custom linter using nogo, here: https://github.com/kubevirt/kubevirt/pull/8051

I think it's nice, but has some learning crave.

One downside (as I see it), is that this is running for each package. Tools like golang-lint provide full report for the whole code. Here I need fix the code, then run again until everything is fixed.

Nahshon.

[Roman Mohr]

On Tue, Jul 5, 2022 at 7:51 PM Nahshon Unna Tsameret <nunn...@redhat.com> wrote:

Hi.

I'm working on a custom linter using nogo, here: https://github.com/kubevirt/kubevirt/pull/8051

It looks great. Thanks for trying it out.

 

I think it's nice, but has some learning crave.

Let me just note that you are doing much more than just adding an existing linter. You are creating one the same way like golangci-linters are done.

One downside (as I see it), is that this is running for each package. Tools like golang-lint provide full report for the whole code. Here I need fix the code, then run again until everything is fixed.

Noted. 

Thank you for your feedback. :)

Roman
 

To view this discussion on the web visit https://groups.google.com/d/msgid/kubevirt-dev/283399e7-0114-44ab-893b-5ac294575d60n%40googlegroups.com.

[Roman Mohr]

Great, let's try things out with that.

There is one caveat right now: For your initial experiment I would pull in https://github.com/kubevirt/kubevirt/pull/8054 to get the, by me, promised nogo integration experience.

This is just temporary, the PR is ready and just needs to get in.

Then the flow should be like this:

 * (temporary): pull in changes from https://github.com/kubevirt/kubevirt/pull/8054
 * add an import like _ "github.com/kisielk/errcheck/errcheck" to "tools/analyzers/analyzer.go" so that we have a reference in the code

 * go get github.com/kisielk/errcheck@latest
 * make dep-sync
 * add "//vendor/github.com/kisielk/errcheck/errcheck:go_default_library" to the root BUILD.bazel nogo config to let nogo know that you want to run the linter
 * add excludes for paths which you don't want to check to nogo.json. Something like this:

```
  "errcheck": {
    "exclude_files": {
      "vendor/": "vendor doesn't pass errcheck",
      "external/": "external doesn't pass errcheck",
      "src/": "golang core code does not pass errcheck"
    }
  }
```

When then running `make` you should see errors popping up.

Nashon already pointed out that fixing code initially when adding a new linter may be a little bit cumbersome, since it will not show you all linter validations at once.

I think it definitely makes sense to, as a release engineer, run the golangci-metalinter with errcheck to get the initial list of things to fix immediately.
I would also recommend to just add one linter after the other and just clean up all reports in one pass instead of stretching the work out too much. That will save you and all others a lot of time.

For me this is a reasonable one-time work to then all people safe from the round-trips of being caught by a dedicated linter on each PR.

Especially considering the almost-zero overhead of running nogo during normal compilation.

But looking forward to your feedback.

Best regards,

[Dan Kenigsberg]

On Tue, Jul 5, 2022 at 1:41 PM Daniel Hiller <dhi...@redhat.com> wrote:

 <snip>

To be clear: I find the initiative to raise code quality to be a good thing, while at the same time I disagree with how it's done.

"THE BOY SCOUTS HAVE A RULE: “Always leave the campground cleaner than you found it.” " (Uncle Bob)

https://www.oreilly.com/library/view/97-things-every/9780596809515/ch08.html#:~:text=Robert%20C.,the%20next%20group%20of%20campers.

From my interpretation of this rule, what should happen should be this:

* you want to implement a feature

* you go to the place where you need to change the code

* you clean up

* you do your changes to implement a feature

* you run all sanity checks locally

* you post your PR

While it's fine to just clean up the code, I think the most value is created when adding new features. In the end the clean up of code should be an integral part of how we add new features.

The boy-scout rule is good, but not good enough when the so-called "broken windows" syndrome is manifested. Nobody thinks about sweeping the pavement on streets where the windows are broken. Our tests/utils.go file is such an example. With its generic name, length and lack of guiding principle, it attracts irregular code. It detracts test writers from thinking about proper packaging and APIs, leading to lack of abstraction and unnecessary complexity. Having a "utils" package is a well-known anti-pattern, which we should address intentionally and methodically, not only as a by-product of other actions.

Regards,

Dan.

[Roman Mohr]

On Tue, Jul 5, 2022 at 3:04 PM Edward Haas <edw...@redhat.com> wrote:

On Tue, Jul 5, 2022 at 3:24 PM Daniel Hiller <dhi...@redhat.com> wrote:

On Tue, Jul 5, 2022 at 1:41 PM Edward Haas <edw...@redhat.com> wrote:

On Tue, Jul 5, 2022 at 1:41 PM Daniel Hiller <dhi...@redhat.com> wrote:

On Tue, Jul 5, 2022 at 11:40 AM Edward Haas <edw...@redhat.com> wrote:

On Tue, Jul 5, 2022 at 11:52 AM Daniel Hiller <dhi...@redhat.com> wrote:

OK, I'll try to go back a step here.

TBH I think that the whole flow of a separate CI lint lane is not a good experience, because:

1) it catches things too late

2) it is a separate step

3) it is another tool

Regarding 1) and 2)

The lint lane kicks in when a commit to a PR is pushed. From my experience people for many reasons do not or do forget to run a linter that is not integrated into the local build process. This produces a high probability of linting errors getting caught too late, while at the same time wasting CI resources that would not have been needed if the linter was run earlier (i.e. even before the local commit was done). IIUC nogo is integrated into the build for exactly that reason. If people see fit that golangci-lint should be required, it should be integrated into building the project. This way people unfamiliar to the project will not wonder about what all that linting stuff is about and still obey the general rules. Please consider that not all developers might be completely familiar with why linting is a good practice in the development process.

The same reasoning presented here is true for unit tests as well.

I disagree. Just running `make bazel-test` instead of `make bazel-build` solves this. So the integration is there, while it's not for `make lint`. I assumed that the latter might be intended, which implied that it runs mostly on CI, which I disagree with, as stated above.

This is exactly how we can solve this point with the linter when it is not embedded in the build.

You add it to an existing target as a dependency.

And as said before, nogo is already integrated into the bazel build process. The solution you are suggesting is not the same, as IIUC this would be added as a make dependency, while an integration on the bazel level would be favorable.

I find the cache part the meaningful advantage, not the fact that it is already wired into bazel build flow.

I am not sure what to say to that. First bazel does also caching local. Second, because you don't seem to care about catching errors early during your development justifies adding additional burden to others?

 

If we look at the usage/call, adding it to a make target has the same result.

It does not. I already shared what it takes on an initial run on my high-end machine to populate the golanci linter caches when executed on the whole codebase. Also that just checking a small subset of the code in CI takes already more than 3 minutes is a hint that on slower machines this will be a huge thing.

It can probably take minutes on slower machines.

I also shared the number of how much it takes to run it once it is cached (6-7s), doubling the time of a `make` invocation on my machine. I already said that I think that invoking `make` already takes too long. It can be made shorter, but definitely not with your suggestion.

 

 

As a contributor, I would like to get service from the CI, no to be forced to calculate my moves and standards based on its load.

If the CI has limitations, I expect it to manage the workloads differently and to fit the usage needs (not the other way around).

Here I am not saying that we should always be aware of CI, I just wanted to make the implication clear. Yes, service from CI should be expected, but there are no unlimited CI resources, which is why one cannot expect this. CI is there to catch things the developer did miss (and not because out of lazyness, that is NOT what CI is for), and if a system can catch things locally before going into a PR, then it should be in place.

While I am accustomed to run locally common targets, I recognize that there are reasons

for contributors not running something locally.

What reasons exactly do you refer to here?

There are the non-intentional ones, e.g.:
- Forgot to run it.
- Run it on the wrong branch/folder.
- Have other files locally that have not been committed,

- Have files removed that have not been committed.

I don't get the point.
No harm is done when they do this non-intentional. Except if they would create a huge amount of PRs which we can't handle, but then it would not matter if they ran it before or not.

And there are intentional ones, e.g.:
- Unable to run it locally temporarily due to some setup/OS problem (e.g. docker crashes). 

- Some devs use remote machines to run their targets, these may be inaccessible due to outage, maintenance or other disruptions.

- Some will like to contribute to less powerful computers (e.g. students) which cannot run some targets.

During that time they can develop in an inefficient way. As said, no one is forced to work the recommended way.

In general your recommendations seem to be a little bit contradictory:

 * On one hand you are hinting below that you only accept suggestions as reasonable that you are forced to follow, so we should force you
 * On the other hand you point out cases where people may not be able to contribute then

I am asking to be mindful, to continue having an open and inclusive community.

 

Moreover, running locally is not an assurance it will pass CI (which runs with a different config in most cases).

No, it's not, mostly because tests are flaky, but that is something we are trying to solve. BTW what different configs do you mean? The bazel cache? The image cache? What else am I overlooking here?

Different CPU architecture, core count, core speed, kernel version, disk speed, disk speed, etc...

Having leftover cache locally can also affect it, including non committed work that can be forgotten.

No one sait that running tests locally is a 100% guarantee that it passes CI. However, you have an extremely high guarantee that things will also pass in CI.

You certainly don't have to run any tests locally. And I also don't run hundreds of tests on every PR locally (but there are cases where I have to), often not a single test.

However, it is a tool to help you iterate very very fast. If you choose not to, you will need, and I think we see that, weeks or months even for simple changes since CI catches a lot of issues early, forcing one to re-evaluate.

And that we catch all that at this stage is for the benefit of devs users and customers.

 

However one decides to look at this, I think that assuming anything about the usage is not safe and therefore undesirable.

I can't follow that reasoning? Can you elaborate? We are talking about developers here using tools, where each one has a certain usage, and if you aren't using it that way, you are holding it wrong. 

You may consider the developer using it wrong or not using it at all, but a CI system cannot assume anything about the usage.

It is up to the CI system to assure that what it examines is correct, anything else, including local runs are not trusted.

In other words, what counts is what the CI says, that is the only thing you can trust.

This is not an anonymous CI system. You are effectively fighting real people, stealing time from real people who have to come up with some plans to cope with your misuse.

This is not a public anonymous cloud provider where you can just take whatever you get until your credit card is exhausted.

 

 

If the development flow can enforce a behavior, then assumptions of usage are reasonable.

I do not think we are in that position at the moment.

Please elaborate on this one.

If you can force the developer to perform a task and only then to push to the CI, only then you can trust it was done.

I do not know how you can do that really.

Expecting and hoping contributors followed the project recommendation before pushing is not something you can assume IMO.

As said before, you can insist on non-optimal development flows. I don't see any reason to force anyone to work in a special way.

However, I don't see a reason to ever change code and not build it locally first. I am also not sure how I should "force" people to build their code locally first or why it should be a strict requirement.

KubeVirt offers a very efficient way of developing which you are free to make use of, or not.

What I think is non-negotiable is that even if you disagree with that way, you have to accept that it exists and that you have to take care and respect the needs and constraints.

Otherwise we would really have to add hard stops or have to enforce flows (which I want to avoid at all costs), and which I think is really the end of improvement.

Best regards,

Roman

[Nahshon Unna Tsameret]

Hi again. Still about my PR (https://github.com/kubevirt/kubevirt/pull/8051)

I think it's now ready for review.

Another problem I found during the development of it is that nogo does not check the /tests directory - or at least the new linter didn't (the tests dir is not in the exclude_files list).

This is a problem for a linter that only deals with test code, like the ginkgo-linter. So right now it only checks for unit tests.

What we can do is to add a CLI code for it, and run it as a command line linter (adding a CLI code is supported by go/analysis - it's a one line main code).

Then it will be maybe make more sense to move the linter to a new repo, and release it.

Nahshon.

[Miguel Duarte de Mora Barroso]

These instructions were *spot on*.

I've integrated the errcheck linter into nogo in PR https://github.com/kubevirt/kubevirt/pull/8058.

Took me 5/10 minutes to follow Roman's instructions and start seeing results.

... then it took me 2 hours to remove the lint errors. 

As Nahshon said in the thread, only seeing errors from *a single* pkg can be frustrating (probably seeing 10000000 errors to fix at once would be worse ...).

I felt like a hamster in a wheel trying to fix those.

Overall, I had a good experience, it was definitely *a lot* easier than I thought it would be.

[Roman Mohr]

On Wed, Jul 6, 2022 at 11:59 AM Nahshon Unna Tsameret <nunn...@redhat.com> wrote:

Hi again. Still about my PR (https://github.com/kubevirt/kubevirt/pull/8051)

I think it's now ready for review.

Another problem I found during the development of it is that nogo does not check the /tests directory - or at least the new linter didn't (the tests dir is not in the exclude_files list).

This is a problem for a linter that only deals with test code, like the ginkgo-linter. So right now it only checks for unit tests.

You may not have seen my comment https://github.com/kubevirt/kubevirt/pull/8051#issuecomment-1175963227. When running `make build-functest` you would see them.

One property of nogo, which makes it by the way so efficient, is that it is applied during the compile time. So if something does not get compiled it does not get checked.

The functest binary is not built right now when running `make`. Only when running `make build-functest` This may be over-optimized, and since many changes in our code-base include changes in `/tests`, it probably makes sense to change that.

As you can see, the e2e test lines compile it, and there you see the errors then.

Will comment on the PR.

Thanks,

Roman

To view this discussion on the web visit https://groups.google.com/d/msgid/kubevirt-dev/bc9b603d-c5f7-4040-964f-d4751d401f8bn%40googlegroups.com.

[Roman Mohr]

Depending on your taste it may make sense to run the golangci-lint binary to initially get a big list to iterate on it faster. I definitely see it that way that we get the big benefits after the initial linter integration is done.

 

Overall, I had a good experience, it was definitely *a lot* easier than I thought it would be.

Thanks for  trying it out.

Roman

 

[Edward Haas]

On Wed, Jul 6, 2022 at 11:24 AM Roman Mohr <rm...@google.com> wrote:

On Tue, Jul 5, 2022 at 3:04 PM Edward Haas <edw...@redhat.com> wrote:

On Tue, Jul 5, 2022 at 3:24 PM Daniel Hiller <dhi...@redhat.com> wrote:

On Tue, Jul 5, 2022 at 1:41 PM Edward Haas <edw...@redhat.com> wrote:

On Tue, Jul 5, 2022 at 1:41 PM Daniel Hiller <dhi...@redhat.com> wrote:

On Tue, Jul 5, 2022 at 11:40 AM Edward Haas <edw...@redhat.com> wrote:

On Tue, Jul 5, 2022 at 11:52 AM Daniel Hiller <dhi...@redhat.com> wrote:

OK, I'll try to go back a step here.

TBH I think that the whole flow of a separate CI lint lane is not a good experience, because:

1) it catches things too late

2) it is a separate step

3) it is another tool

Regarding 1) and 2)

The lint lane kicks in when a commit to a PR is pushed. From my experience people for many reasons do not or do forget to run a linter that is not integrated into the local build process. This produces a high probability of linting errors getting caught too late, while at the same time wasting CI resources that would not have been needed if the linter was run earlier (i.e. even before the local commit was done). IIUC nogo is integrated into the build for exactly that reason. If people see fit that golangci-lint should be required, it should be integrated into building the project. This way people unfamiliar to the project will not wonder about what all that linting stuff is about and still obey the general rules. Please consider that not all developers might be completely familiar with why linting is a good practice in the development process.

The same reasoning presented here is true for unit tests as well.

I disagree. Just running `make bazel-test` instead of `make bazel-build` solves this. So the integration is there, while it's not for `make lint`. I assumed that the latter might be intended, which implied that it runs mostly on CI, which I disagree with, as stated above.

This is exactly how we can solve this point with the linter when it is not embedded in the build.

You add it to an existing target as a dependency.

And as said before, nogo is already integrated into the bazel build process. The solution you are suggesting is not the same, as IIUC this would be added as a make dependency, while an integration on the bazel level would be favorable.

I find the cache part the meaningful advantage, not the fact that it is already wired into bazel build flow.

I am not sure what to say to that. First bazel does also caching local. Second, because you don't seem to care about catching errors early during your development justifies adding additional burden to others?

Per my understanding, local cache is done by both lint runners.

Assuming that I care or not is not part of this technical discussion, I would prefer to stick to facts.

Catching the errors is about when one calls a specific target and that I classified as a different topic then the cache.

 

If we look at the usage/call, adding it to a make target has the same result.

It does not. I already shared what it takes on an initial run on my high-end machine to populate the golanci linter caches when executed on the whole codebase. Also that just checking a small subset of the code in CI takes already more than 3 minutes is a hint that on slower machines this will be a huge thing.

It can probably take minutes on slower machines.

I also shared the number of how much it takes to run it once it is cached (6-7s), doubling the time of a `make` invocation on my machine. I already said that I think that invoking `make` already takes too long. It can be made shorter, but definitely not with your suggestion.

The golangci-lint is currently set to run 37 linters, each with multiple functionalities it covers.

I am not really clear how a comparison between runners can be compared when they do not run the same thing.

It seems to me that we can compare them only when they run similar things.

While one can provide a target to run what is considered basic, there is still freedom to run whatever one thinks is relevant.

As an example, a developer may choose to use an IDE that runs all these linters automatically while coding and therefore has no

need to run this target explicitly.

Requiring to run a linter as part of a build is an option that comes with a cost, it is not free.

I do not see the difference between nogo and other linter runners in this regard. If there is one, please help me understand what it is.

 

 

As a contributor, I would like to get service from the CI, no to be forced to calculate my moves and standards based on its load.

If the CI has limitations, I expect it to manage the workloads differently and to fit the usage needs (not the other way around).

Here I am not saying that we should always be aware of CI, I just wanted to make the implication clear. Yes, service from CI should be expected, but there are no unlimited CI resources, which is why one cannot expect this. CI is there to catch things the developer did miss (and not because out of lazyness, that is NOT what CI is for), and if a system can catch things locally before going into a PR, then it should be in place.

While I am accustomed to run locally common targets, I recognize that there are reasons

for contributors not running something locally.

What reasons exactly do you refer to here?

There are the non-intentional ones, e.g.:
- Forgot to run it.
- Run it on the wrong branch/folder.
- Have other files locally that have not been committed,

- Have files removed that have not been committed.

I don't get the point.
No harm is done when they do this non-intentional. Except if they would create a huge amount of PRs which we can't handle, but then it would not matter if they ran it before or not.

I elaborated on reasons on why one will not run it, as requested.

And there are intentional ones, e.g.:
- Unable to run it locally temporarily due to some setup/OS problem (e.g. docker crashes). 

- Some devs use remote machines to run their targets, these may be inaccessible due to outage, maintenance or other disruptions.

- Some will like to contribute to less powerful computers (e.g. students) which cannot run some targets.

During that time they can develop in an inefficient way. As said, no one is forced to work the recommended way.

Great.

In general your recommendations seem to be a little bit contradictory:

 * On one hand you are hinting below that you only accept suggestions as reasonable that you are forced to follow, so we should force you
 * On the other hand you point out cases where people may not be able to contribute then

I am asking to be mindful, to continue having an open and inclusive community.

 

Moreover, running locally is not an assurance it will pass CI (which runs with a different config in most cases).

No, it's not, mostly because tests are flaky, but that is something we are trying to solve. BTW what different configs do you mean? The bazel cache? The image cache? What else am I overlooking here?

Different CPU architecture, core count, core speed, kernel version, disk speed, disk speed, etc...

Having leftover cache locally can also affect it, including non committed work that can be forgotten.

No one sait that running tests locally is a 100% guarantee that it passes CI. However, you have an extremely high guarantee that things will also pass in CI.

You certainly don't have to run any tests locally. And I also don't run hundreds of tests on every PR locally (but there are cases where I have to), often not a single test.

However, it is a tool to help you iterate very very fast. If you choose not to, you will need, and I think we see that, weeks or months even for simple changes since CI catches a lot of issues early, forcing one to re-evaluate.

And that we catch all that at this stage is for the benefit of devs users and customers.

 

However one decides to look at this, I think that assuming anything about the usage is not safe and therefore undesirable.

I can't follow that reasoning? Can you elaborate? We are talking about developers here using tools, where each one has a certain usage, and if you aren't using it that way, you are holding it wrong. 

You may consider the developer using it wrong or not using it at all, but a CI system cannot assume anything about the usage.

It is up to the CI system to assure that what it examines is correct, anything else, including local runs are not trusted.

In other words, what counts is what the CI says, that is the only thing you can trust.

This is not an anonymous CI system. You are effectively fighting real people, stealing time from real people who have to come up with some plans to cope with your misuse.

This is not a public anonymous cloud provider where you can just take whatever you get until your credit card is exhausted.

Maybe we should consider adjusting the CI flow to how it is used and not attempting to lecture its users on how they should behave.

The discussion is focusing on expectations, and my expectations are to realize developers behave differently for various reasons.

If there is a problem with the CI, I expect it to adjust and not forward the responsibility on to its users.

[Miguel Duarte de Mora Barroso]

The PR that integrated `errcheck` into nogo was merged yesterday.

From now on, on particular pkgs in the code (mostly test related), it'll fail the `make` target if you do not handle the returned error.

My plan is to gradually increase the coverage of the plugin in the kubevirt repo, until it eventually covers all the KubeVirt code. 

You can infer which pkgs are being linted by checking the nogo configuration file, located in

https://github.com/kubevirt/kubevirt/blob/main/nogo_config.json#L144

[Fabian Deutsch]

Yo!

Where did we actually end up here?

Did we introduce a linter to avoid the growth of utils go?

- fabian

To view this discussion on the web visit https://groups.google.com/d/msgid/kubevirt-dev/CAAx_ZVXXATqxHgtUT-WMJLFOdcM1oaKLdy6QJ6k7tum3NoSOjw%40mail.gmail.com.

[Edward Haas]

On Thu, Aug 18, 2022 at 2:41 PM Fabian Deutsch <fdeu...@redhat.com> wrote:

Yo!

Where did we actually end up here?

We have a few linters which have been introduced into nogo (like the `errcheck` from Miguel and the `ginkgo/gomega` from Nahshon).

The ones from `golangci-lint` (30+ linters) are available, but are not blocking PR/s on CI (there is no consensus unfortunately).

Did we introduce a linter to avoid the growth of utils go?

There is in place Dan's simple oneliner lines count, the more advanced one (that does the same for all the files) has some opposition.

[Miguel Duarte de Mora Barroso]

On Thu, Aug 18, 2022 at 2:03 PM Edward Haas <edw...@redhat.com> wrote:

On Thu, Aug 18, 2022 at 2:41 PM Fabian Deutsch <fdeu...@redhat.com> wrote:

Yo!

Where did we actually end up here?

We have a few linters which have been introduced into nogo (like the `errcheck` from Miguel and the `ginkgo/gomega` from Nahshon).

I'd like to take the opportunity to let the community know that PRs to remove packages from `errcheck`s ignore list are welcome! 

Just remove stuff from the nogo errcheck configure list, and start compiling... You'll be pointed to the errors.

The nogo configuration is found in https://github.com/kubevirt/kubevirt/blob/main/nogo_config.json#L144

Please refer to https://github.com/kubevirt/kubevirt/pull/8259 for an example PR on how to do that. Reviews are welcome :) 

[Fabian Deutsch]

On Thu, Aug 18, 2022 at 2:03 PM Edward Haas <edw...@redhat.com> wrote:

On Thu, Aug 18, 2022 at 2:41 PM Fabian Deutsch <fdeu...@redhat.com> wrote:

Yo!

Where did we actually end up here?

We have a few linters which have been introduced into nogo (like the `errcheck` from Miguel and the `ginkgo/gomega` from Nahshon).

The ones from `golangci-lint` (30+ linters) are available, but are not blocking PR/s on CI (there is no consensus unfortunately).

If we limit this to the one (and ignoring the remaining 29+ ones for now) linter which is checking the line length, would this find agreement?

[Edward Haas]

On Thu, Aug 18, 2022 at 3:53 PM Fabian Deutsch <fdeu...@redhat.com> wrote:

On Thu, Aug 18, 2022 at 2:03 PM Edward Haas <edw...@redhat.com> wrote:

On Thu, Aug 18, 2022 at 2:41 PM Fabian Deutsch <fdeu...@redhat.com> wrote:

Yo!

Where did we actually end up here?

We have a few linters which have been introduced into nogo (like the `errcheck` from Miguel and the `ginkgo/gomega` from Nahshon).

The ones from `golangci-lint` (30+ linters) are available, but are not blocking PR/s on CI (there is no consensus unfortunately).

If we limit this to the one (and ignoring the remaining 29+ ones for now) linter which is checking the line length, would this find agreement?

It's the line count, not the line length (the one that does line length is called `lll` and is part of the others).

The ones that come with `golangci-lint` do not include the line count.

The line count one came as a new linter written by Nahshon and targeted to be added to nogo.

The disagreement is about the limit itself (of line count), not about which runner should it be included in.

There is a bit of a mess, hope it is clearer now.

[Fabian Deutsch]

On Thu, Aug 18, 2022 at 3:03 PM Edward Haas <edw...@redhat.com> wrote:

On Thu, Aug 18, 2022 at 3:53 PM Fabian Deutsch <fdeu...@redhat.com> wrote:

On Thu, Aug 18, 2022 at 2:03 PM Edward Haas <edw...@redhat.com> wrote:

On Thu, Aug 18, 2022 at 2:41 PM Fabian Deutsch <fdeu...@redhat.com> wrote:

Yo!

Where did we actually end up here?

We have a few linters which have been introduced into nogo (like the `errcheck` from Miguel and the `ginkgo/gomega` from Nahshon).

The ones from `golangci-lint` (30+ linters) are available, but are not blocking PR/s on CI (there is no consensus unfortunately).

If we limit this to the one (and ignoring the remaining 29+ ones for now) linter which is checking the line length, would this find agreement?

It's the line count, not the line length

Sorry, my bad, clearly, yes

 

(the one that does line length is called `lll` and is part of the others).

The ones that come with `golangci-lint` do not include the line count.

The line count one came as a new linter written by Nahshon and targeted to be added to nogo.

Are we confident that this will land in the upstream project and will be consumable by us?

 

The disagreement is about the limit itself (of line count), not about which runner should it be included in.

Can the disagreement be summairzed?

What about the naive approach to use the line length of -1 month from day.

 

There is a bit of a mess, hope it is clearer now.

Thanks, it helped

[Edward Haas]

On Thu, Aug 18, 2022 at 4:09 PM Fabian Deutsch <fdeu...@redhat.com> wrote:

On Thu, Aug 18, 2022 at 3:03 PM Edward Haas <edw...@redhat.com> wrote:

On Thu, Aug 18, 2022 at 3:53 PM Fabian Deutsch <fdeu...@redhat.com> wrote:

On Thu, Aug 18, 2022 at 2:03 PM Edward Haas <edw...@redhat.com> wrote:

On Thu, Aug 18, 2022 at 2:41 PM Fabian Deutsch <fdeu...@redhat.com> wrote:

Yo!

Where did we actually end up here?

We have a few linters which have been introduced into nogo (like the `errcheck` from Miguel and the `ginkgo/gomega` from Nahshon).

The ones from `golangci-lint` (30+ linters) are available, but are not blocking PR/s on CI (there is no consensus unfortunately).

If we limit this to the one (and ignoring the remaining 29+ ones for now) linter which is checking the line length, would this find agreement?

It's the line count, not the line length

Sorry, my bad, clearly, yes

 

(the one that does line length is called `lll` and is part of the others).

The ones that come with `golangci-lint` do not include the line count.

The line count one came as a new linter written by Nahshon and targeted to be added to nogo.

Are we confident that this will land in the upstream project and will be consumable by us?

 

The disagreement is about the limit itself (of line count), not about which runner should it be included in.

Can the disagreement be summairzed?

https://github.com/kubevirt/kubevirt/pull/8085

What about the naive approach to use the line length of -1 month from day.

Not sure what that means.

This is what exists today: https://github.com/kubevirt/kubevirt/blob/84be129e98ce39ce33cc7068f68460ac3400e533/Makefile#L192

And it is running on each PR change (CI), but it is not a required check.

The disagreement on making that lint job as required, is affecting this lint as well.

[Daniel Hiller]

I don't want to get into too much detail here but we disagreed about having a separate linter job vs. using nogo which is already there and being executed on every make build anyway. Thus the linter job is not yet required which I find acceptable. Also the fact that it was failing for a long time without anyone seemingly feeling responsible to fix it put me off from accepting this as the way to go. But that might just be my view with which others disagree.

I think we agreed on the fact to leave the job and make target for people to explore the several linters that it offers, but focus on integrating the linting into nogo. I think Nashon and Miguel (and maybe others that I forgot to mention) already contributed to the latter.

--

-- 

Best,

Daniel

[Edward Haas]

On Fri, Aug 19, 2022 at 9:53 AM Daniel Hiller <dhi...@redhat.com> wrote:

I don't want to get into too much detail here but we disagreed about having a separate linter job vs. using nogo which is already there and being executed on every make build anyway.

I find it odd to compare them and raise it as if one is an alternative to the other at this stage.

Per the existing state, only a very few linters are run using nogo and a large amount are available using the other.

Thus the linter job is not yet required which I find acceptable.

Also the fact that it was failing for a long time without anyone seemingly feeling responsible to fix it put me off from accepting this as the way to go. But that might just be my view with which others disagree.

I do not think this is sustainable.

On one hand it is acceptable to keep it optional, on the other hand there is expectation for it to stay green.
In practice, some maintainers ignore the optional job linter result and therefore are likely to break it.

I disagree with the argument of having no owner. I have an interest for it to run and keep the covered code clean and I think the job [1] was

green for a large period of time already.

[1] https://prow.ci.kubevirt.io/job-history/gs/kubevirt-prow/pr-logs/directory/pull-kubevirt-code-lint

 

I think we agreed on the fact to leave the job and make target for people to explore the several linters that it offers, but focus on integrating the linting into nogo. I think Nashon and Miguel (and maybe others that I forgot to mention) already contributed to the latter.

Perhaps it is worth checking with them if doing the same for 30+ linters makes sense and what is the actual cost to maintain that.

I am not against having it run on nogo, I just find the maintenance cost high and the gain low.

I find it odd that linters are controversial, especially when they are embraced by a large amount of projects to increase code quality.

I am not sure for how long we can keep the optional linters, at some point we will either remove them, make them required or embed them all into nogo.

[Roman Mohr]

On Fri, Aug 19, 2022 at 9:27 AM Edward Haas <edw...@redhat.com> wrote:

On Fri, Aug 19, 2022 at 9:53 AM Daniel Hiller <dhi...@redhat.com> wrote:

I don't want to get into too much detail here but we disagreed about having a separate linter job vs. using nogo which is already there and being executed on every make build anyway.

I find it odd to compare them and raise it as if one is an alternative to the other at this stage.

Per the existing state, only a very few linters are run using nogo and a large amount are available using the other.

Thus the linter job is not yet required which I find acceptable.

Also the fact that it was failing for a long time without anyone seemingly feeling responsible to fix it put me off from accepting this as the way to go. But that might just be my view with which others disagree.

I do not think this is sustainable.

On one hand it is acceptable to keep it optional, on the other hand there is expectation for it to stay green.
In practice, some maintainers ignore the optional job linter result and therefore are likely to break it.

I disagree with the argument of having no owner. I have an interest for it to run and keep the covered code clean and I think the job [1] was

green for a large period of time already.

[1] https://prow.ci.kubevirt.io/job-history/gs/kubevirt-prow/pr-logs/directory/pull-kubevirt-code-lint

 

I think we agreed on the fact to leave the job and make target for people to explore the several linters that it offers, but focus on integrating the linting into nogo. I think Nashon and Miguel (and maybe others that I forgot to mention) already contributed to the latter.

Perhaps it is worth checking with them if doing the same for 30+ linters makes sense and what is the actual cost to maintain that.

I am not against having it run on nogo, I just find the maintenance cost high and the gain low.

I don't see any reason to just blindly enable 30+ linters and then uber-optimize 2 files in the project while the rest did not even start using the first additional linter.

Best regards,

Roman 

[Fabian Deutsch]

On Fri, Aug 19, 2022 at 9:27 AM Edward Haas <edw...@redhat.com> wrote:

On Fri, Aug 19, 2022 at 9:53 AM Daniel Hiller <dhi...@redhat.com> wrote:

I don't want to get into too much detail here but we disagreed about having a separate linter job vs. using nogo which is already there and being executed on every make build anyway.

I find it odd to compare them and raise it as if one is an alternative to the other at this stage.

Per the existing state, only a very few linters are run using nogo and a large amount are available using the other.

Thus the linter job is not yet required which I find acceptable.

Also the fact that it was failing for a long time without anyone seemingly feeling responsible to fix it put me off from accepting this as the way to go. But that might just be my view with which others disagree.

I do not think this is sustainable.

On one hand it is acceptable to keep it optional, on the other hand there is expectation for it to stay green.
In practice, some maintainers ignore the optional job linter result and therefore are likely to break it.

I disagree with the argument of having no owner. I have an interest for it to run and keep the covered code clean and I think the job [1] was

green for a large period of time already.

[1] https://prow.ci.kubevirt.io/job-history/gs/kubevirt-prow/pr-logs/directory/pull-kubevirt-code-lint

 

I think we agreed on the fact to leave the job and make target for people to explore the several linters that it offers, but focus on integrating the linting into nogo. I think Nashon and Miguel (and maybe others that I forgot to mention) already contributed to the latter.

Perhaps it is worth checking with them if doing the same for 30+ linters makes sense and what is the actual cost to maintain that.

I am not against having it run on nogo, I just find the maintenance cost high and the gain low.

Edward, IIRC then Nahshon provided a PR against nogog in order to introduce the line length linter. Has this been merged, can we merge this?

In this first step I'd focus on seeing that we can enable this (agreed) line length linter.

And then we have a wonderful opportunity to look and evaluate what other linters  - out of the 30 curated ones - we should enable.

To me the benefit is: We make a step into the direction which at least allows us to (as so often:) stand on the shoulders of giants and benefit from the other linters in nogo as well.

[Edward Haas]

On Mon, Sep 5, 2022 at 3:37 PM Fabian Deutsch <fdeu...@redhat.com> wrote:

On Fri, Aug 19, 2022 at 9:27 AM Edward Haas <edw...@redhat.com> wrote:

On Fri, Aug 19, 2022 at 9:53 AM Daniel Hiller <dhi...@redhat.com> wrote:

I don't want to get into too much detail here but we disagreed about having a separate linter job vs. using nogo which is already there and being executed on every make build anyway.

I find it odd to compare them and raise it as if one is an alternative to the other at this stage.

Per the existing state, only a very few linters are run using nogo and a large amount are available using the other.

Thus the linter job is not yet required which I find acceptable.

Also the fact that it was failing for a long time without anyone seemingly feeling responsible to fix it put me off from accepting this as the way to go. But that might just be my view with which others disagree.

I do not think this is sustainable.

On one hand it is acceptable to keep it optional, on the other hand there is expectation for it to stay green.
In practice, some maintainers ignore the optional job linter result and therefore are likely to break it.

I disagree with the argument of having no owner. I have an interest for it to run and keep the covered code clean and I think the job [1] was

green for a large period of time already.

[1] https://prow.ci.kubevirt.io/job-history/gs/kubevirt-prow/pr-logs/directory/pull-kubevirt-code-lint

 

I think we agreed on the fact to leave the job and make target for people to explore the several linters that it offers, but focus on integrating the linting into nogo. I think Nashon and Miguel (and maybe others that I forgot to mention) already contributed to the latter.

Perhaps it is worth checking with them if doing the same for 30+ linters makes sense and what is the actual cost to maintain that.

I am not against having it run on nogo, I just find the maintenance cost high and the gain low.

Edward, IIRC then Nahshon provided a PR against nogog in order to introduce the line length linter. Has this been merged, can we merge this?

This was discussed in this thread (at Aug 18, 2022, 6:45 PM). There was a disagreement and it did not get in.

In this first step I'd focus on seeing that we can enable this (agreed) line length linter.

And then we have a wonderful opportunity to look and evaluate what other linters  - out of the 30 curated ones - we should enable.

To me the benefit is: We make a step into the direction which at least allows us to (as so often:) stand on the shoulders of giants and benefit from the other linters in nogo as well.

Well, I can just claim it is the other way around.

There is little I can do or say at the moment, I just hope, for the project success, that more linters will get in to increase the code quality.

[Fabian Deutsch]

On Mon, Sep 5, 2022 at 3:38 PM Edward Haas <edw...@redhat.com> wrote:

On Mon, Sep 5, 2022 at 3:37 PM Fabian Deutsch <fdeu...@redhat.com> wrote:

On Fri, Aug 19, 2022 at 9:27 AM Edward Haas <edw...@redhat.com> wrote:

On Fri, Aug 19, 2022 at 9:53 AM Daniel Hiller <dhi...@redhat.com> wrote:

I don't want to get into too much detail here but we disagreed about having a separate linter job vs. using nogo which is already there and being executed on every make build anyway.

I find it odd to compare them and raise it as if one is an alternative to the other at this stage.

Per the existing state, only a very few linters are run using nogo and a large amount are available using the other.

Thus the linter job is not yet required which I find acceptable.

Also the fact that it was failing for a long time without anyone seemingly feeling responsible to fix it put me off from accepting this as the way to go. But that might just be my view with which others disagree.

I do not think this is sustainable.

On one hand it is acceptable to keep it optional, on the other hand there is expectation for it to stay green.
In practice, some maintainers ignore the optional job linter result and therefore are likely to break it.

I disagree with the argument of having no owner. I have an interest for it to run and keep the covered code clean and I think the job [1] was

green for a large period of time already.

[1] https://prow.ci.kubevirt.io/job-history/gs/kubevirt-prow/pr-logs/directory/pull-kubevirt-code-lint

 

I think we agreed on the fact to leave the job and make target for people to explore the several linters that it offers, but focus on integrating the linting into nogo. I think Nashon and Miguel (and maybe others that I forgot to mention) already contributed to the latter.

Perhaps it is worth checking with them if doing the same for 30+ linters makes sense and what is the actual cost to maintain that.

I am not against having it run on nogo, I just find the maintenance cost high and the gain low.

Edward, IIRC then Nahshon provided a PR against nogog in order to introduce the line length linter. Has this been merged, can we merge this?

This was discussed in this thread (at Aug 18, 2022, 6:45 PM). There was a disagreement and it did not get in.

IIUIC then the disagreement was about where to apply the linter. Is this correct?

Can the PR changed to start with focusing on utils.go? (And not be applied universally)

And I apologize in advance if this is already the case, I was not able to follow every detail.

 

In this first step I'd focus on seeing that we can enable this (agreed) line length linter.

And then we have a wonderful opportunity to look and evaluate what other linters  - out of the 30 curated ones - we should enable.

To me the benefit is: We make a step into the direction which at least allows us to (as so often:) stand on the shoulders of giants and benefit from the other linters in nogo as well.

Well, I can just claim it is the other way around.

There is little I can do or say at the moment, I just hope, for the project success, that more linters will get in to increase the code quality.

Ah, I do think that you can help!

I appreciate that you help to touch upon this topic.

<sniiiiiiiiiiip-all-the-rest />

[Nahshon Unna Tsameret]

On Mon, Sep 5, 2022 at 4:44 PM Fabian Deutsch <fdeu...@redhat.com> wrote:

On Mon, Sep 5, 2022 at 3:38 PM Edward Haas <edw...@redhat.com> wrote:

On Mon, Sep 5, 2022 at 3:37 PM Fabian Deutsch <fdeu...@redhat.com> wrote:

On Fri, Aug 19, 2022 at 9:27 AM Edward Haas <edw...@redhat.com> wrote:

On Fri, Aug 19, 2022 at 9:53 AM Daniel Hiller <dhi...@redhat.com> wrote:

I don't want to get into too much detail here but we disagreed about having a separate linter job vs. using nogo which is already there and being executed on every make build anyway.

I find it odd to compare them and raise it as if one is an alternative to the other at this stage.

Per the existing state, only a very few linters are run using nogo and a large amount are available using the other.

Thus the linter job is not yet required which I find acceptable.

Also the fact that it was failing for a long time without anyone seemingly feeling responsible to fix it put me off from accepting this as the way to go. But that might just be my view with which others disagree.

I do not think this is sustainable.

On one hand it is acceptable to keep it optional, on the other hand there is expectation for it to stay green.
In practice, some maintainers ignore the optional job linter result and therefore are likely to break it.

I disagree with the argument of having no owner. I have an interest for it to run and keep the covered code clean and I think the job [1] was

green for a large period of time already.

[1] https://prow.ci.kubevirt.io/job-history/gs/kubevirt-prow/pr-logs/directory/pull-kubevirt-code-lint

 

I think we agreed on the fact to leave the job and make target for people to explore the several linters that it offers, but focus on integrating the linting into nogo. I think Nashon and Miguel (and maybe others that I forgot to mention) already contributed to the latter.

Perhaps it is worth checking with them if doing the same for 30+ linters makes sense and what is the actual cost to maintain that.

I am not against having it run on nogo, I just find the maintenance cost high and the gain low.

Edward, IIRC then Nahshon provided a PR against nogog in order to introduce the line length linter. Has this been merged, can we merge this?

This was discussed in this thread (at Aug 18, 2022, 6:45 PM). There was a disagreement and it did not get in.

IIUIC then the disagreement was about where to apply the linter. Is this correct?

Can the PR changed to start with focusing on utils.go? (And not be applied universally)

And I apologize in advance if this is already the case, I was not able to follow every detail.

I think there was another discussion about the long file linter - is the number of lines a good enough indication for quality. Sure, long files are code smell, no doubt, but it's just a smell. I think code complexity, provided by tools like sonar cloud, gives better indication.

So I voted against merging my PR...

 

In this first step I'd focus on seeing that we can enable this (agreed) line length linter.

And then we have a wonderful opportunity to look and evaluate what other linters  - out of the 30 curated ones - we should enable.

To me the benefit is: We make a step into the direction which at least allows us to (as so often:) stand on the shoulders of giants and benefit from the other linters in nogo as well.

Well, I can just claim it is the other way around.

There is little I can do or say at the moment, I just hope, for the project success, that more linters will get in to increase the code quality.

Ah, I do think that you can help!

I appreciate that you help to touch upon this topic.

<sniiiiiiiiiiip-all-the-rest />

--
You received this message because you are subscribed to a topic in the Google Groups "kubevirt-dev" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/kubevirt-dev/QSjoptOP9Vo/unsubscribe.
To unsubscribe from this group and all its topics, send an email to kubevirt-dev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubevirt-dev/CA%2BPVUaRNm6s8AAnvcH3VJ2hGzR7P%3D8K_sUdOCYUo4rD8KHSUaQ%40mail.gmail.com.

[Fabian Deutsch]

Ha! Thanks for adding this opinion to this thread which was lacking opinions! :)

Fair. I agree there is no general rule of thumb for all files. And generally no indicator about quality. Let#s just think of hte auto generated files …

However, for some files we - humans - could decide that certain files should not grow in length, simply to set a tombstone, to make it clear to everybody - to those who did not know, who are first time contributors, and so one ..

In this particular example - of this thread - I would like to see if we find an aligned approach of how to stop certain files from growing and second to look at how this can be applied to at least utils.go