Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

logcheck: should not return error if no Go files are present #214

Closed
ehashman opened this issue Mar 4, 2021 · 21 comments
Closed

logcheck: should not return error if no Go files are present #214

ehashman opened this issue Mar 4, 2021 · 21 comments
Assignees
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.

Comments

@ehashman
Copy link
Member

ehashman commented Mar 4, 2021

/kind bug

What steps did you take and what happened:

When running logcheck against a directory that contains no Go files, I get an error:

-: no Go files in /home/ehashman/src/k8s/_output/local/go/src/k8s.io/kubernetes/pkg/kubelet/volumemanager/reconciler/pods
logcheck: error during loading

What did you expect to happen:

I would expect a directory with no Go files to pass the linter.

Anything else you would like to add:

/cc @adisky
/priority important-longterm

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Mar 4, 2021
@adisky
Copy link

adisky commented Mar 8, 2021

/good-first-issue

@k8s-ci-robot
Copy link

@adisky:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/good-first-issue

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Mar 8, 2021
@umangachapagain
Copy link

/assign

@serathius
Copy link

serathius commented Mar 19, 2021

@ehashman is problem mitigated by running linter against parent directories? For example now we run against pkg/kubelet/... which covers pkg/kubelet/volumemanager/reconciler/pods. This way it skips directories without Go files.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 17, 2021
@umangachapagain
Copy link

This issue blocks kubernetes/kubernetes/issues/102439 which is targeted for v1.22.

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 29, 2021
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 27, 2021
@MadhavJivrajani
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 27, 2021
@daemon1024
Copy link

I would like to look into this 🤓 /assign

@RinkiyaKeDad
Copy link
Member

/assign @daemon1024

@shiva1333
Copy link

shiva1333 commented Sep 28, 2021

Hey @daemon1024 can you please give us a rough estimate as in how much time it will require to come up with a fix for this?
I'd suggest you to go through review comments of #220, it will give you an idea about the expected fix.
This is important for kubernetes/kubernetes#103293, if you need any help please feel free to ping us on slack #wg-structured-logging

@shiva1333
Copy link

/priority important-soon

@k8s-ci-robot k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Sep 28, 2021
@shiva1333
Copy link

/remove-priority important-longterm

@k8s-ci-robot k8s-ci-robot removed the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Sep 28, 2021
@daemon1024
Copy link

@shivanshu1333 I have went through the relevant discussions. I am currently in process of setting up and reproducing it. I will have a fix or some concrete update by the end of the week.
Thank You I will make sure to reach out if I face some issues.

@shiva1333
Copy link

That's awesome, thanks!

@daemon1024
Copy link

Hello, Apologies for the delay.

As pointed out in #220 (comment), we can't depend on analyzer main function since it directly exits. So we need our own validation logic.

From #220 (comment)

I suspect the best way to accomplish this may be to always return true if given a path that contains no Go files before starting the rest of the program logic. This would constitute input validation and is not the way the PR is currently written.

So I rewrote the logic in the PR more in line with what this comment suggested. Here's the link to the commit daemon1024@fe26f9b

The caveat here is that we will be loading packages twice.

The other way to handle the validation is, we walk through the directory to check if there are any go files present there or not.

Here's the commit daemon1024@71d4862 using file walk.

I understand sharing commits is not the right way to ask for review but I wanted to confirm which of the methods should I proceed with before raising a PR.

@adisky
Copy link

adisky commented Oct 25, 2021

@daemon1024 Feel free to raise a PR, there is no harm in raising the PR

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 23, 2022
@shiva1333
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 23, 2022
@liggitt
Copy link
Member

liggitt commented Mar 25, 2022

fixed by kubernetes/kubernetes#108159

/close

@k8s-ci-robot
Copy link

@liggitt: Closing this issue.

In response to this:

fixed by kubernetes/kubernetes#108159

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet