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

Add linting based on the elastic-agent-libs for Beats #30545

Closed
cmacknz opened this issue Feb 22, 2022 · 7 comments · Fixed by #30896
Closed

Add linting based on the elastic-agent-libs for Beats #30545

cmacknz opened this issue Feb 22, 2022 · 7 comments · Fixed by #30896
Assignees
Labels
Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team v8.2.0

Comments

@cmacknz
Copy link
Member

cmacknz commented Feb 22, 2022

Now that a base configuration for golangci-lint has been created and setup for elastic-agent-libs we should do the same for the beats repository.

See #30150 for the related PRs.

Since the beats repository is so large we will need to be more careful when rolling it out to ensure the new linters do not slow down the development workflow.

We will likely also only want to have the linter only prevent new issues initially, rather than fixing all existing issues. PRs to address existing issues can be quite large, for example #30483.

See go-libaudit for an example of how to configure the linter to only find new issues: https://github.com/elastic/go-libaudit/blob/227948a588b4c181d09a55154a476372525be193/.github/workflows/golangci-lint.yml#L29-L30

@cmacknz cmacknz self-assigned this Feb 22, 2022
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Feb 22, 2022
@cmacknz cmacknz added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Feb 22, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Feb 22, 2022
@rdner
Copy link
Member

rdner commented Feb 23, 2022

See go-libaudit for an example of how to configure the linter to only find new issues: https://github.com/elastic/go-libaudit/blob/227948a588b4c181d09a55154a476372525be193/.github/workflows/golangci-lint.yml#L29-L30

Just to avoid future problems, this configuration simply does not work. I tested it manually and had to use --new-from-rev and --whole-file additionally to this flag (see https://github.com/elastic/elastic-agent-libs/pull/11/files#diff-520109d035a196d29d0a86e9c4e6c98e98abf056141b84df68c0d48167b87b25R127-R132

  • First of all, we need changes since master, not just new issues
  • Second, the default behaviour of --new-from-rev is supposed to inspect only diffs and seems like it's broken in the linter at the moment since it does not report anything.

@cmacknz cmacknz assigned rdner and unassigned cmacknz Feb 23, 2022
@cmacknz
Copy link
Member Author

cmacknz commented Feb 23, 2022

I assigned this to you @rdner, if you run out of time for it feel free to send it back to me.

@jlind23
Copy link
Collaborator

jlind23 commented Mar 14, 2022

@rdner @cmacknz is this something we made some progress on? Should we postpone it to another release or should we keep it here?

@cmacknz
Copy link
Member Author

cmacknz commented Mar 14, 2022

I think it stalled due to other priorities, I don't think @rdner is actively working on this.

It's not critical to do in 8.2. We can unassign it change the label if we want and someone can take it once they have time for it.

@rdner
Copy link
Member

rdner commented Mar 14, 2022

@jlind23 @cmacknz I was planning to get it done this week, perhaps even tomorrow.

@cmacknz cmacknz assigned cmacknz and rdner and unassigned cmacknz Mar 14, 2022
@cmacknz
Copy link
Member Author

cmacknz commented Mar 14, 2022

Perfect, assigned back to you then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team v8.2.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants