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 rules for replace directives in go.mod #2638

Merged
merged 11 commits into from
May 16, 2023

Conversation

ycombinator
Copy link
Contributor

@ycombinator ycombinator commented May 9, 2023

What does this PR do?

This PR adds a golangci-lint linter rule for monitoring the use of replace directives in go.mod.

Why is it important?

Developers will sometimes use temporary replace directives in go.mod while developing a feature or fixing a bug that requires changes in dependencies as well. The linter rule introduced in this PR will ensure such temporary replace directives do not get checked in by accident, by complaining on the PR like so:

Screenshot 2023-05-09 at 16 35 43

@ycombinator ycombinator requested a review from a team as a code owner May 9, 2023 19:27
@ycombinator ycombinator marked this pull request as draft May 9, 2023 19:28
@mergify
Copy link
Contributor

mergify bot commented May 9, 2023

This pull request does not have a backport label. Could you fix it @ycombinator? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@elasticmachine
Copy link
Contributor

elasticmachine commented May 9, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-05-09T20:36:16.925+0000

  • Duration: 20 min 24 sec

Test stats 🧪

Test Results
Failed 0
Passed 5651
Skipped 23
Total 5674

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages.

  • run integration tests : Run the Elastic Agent Integration tests.

  • run end-to-end tests : Generate the packages and run the E2E Tests.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@elasticmachine
Copy link
Contributor

elasticmachine commented May 9, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 98.592% (70/71) 👍
Files 68.571% (168/245) 👍
Classes 67.532% (312/462) 👍
Methods 53.794% (950/1766) 👍
Lines 39.318% (10818/27514) 👍 0.044
Conditionals 100.0% (0/0) 💚

@ycombinator ycombinator added backport-v8.8.0 Automated backport with mergify Team:Elastic-Agent Label for the Agent team and removed backport-skip labels May 9, 2023
@ycombinator ycombinator marked this pull request as ready for review May 9, 2023 20:37
@ycombinator ycombinator added Team:Elastic-Agent Label for the Agent team and removed Team:Elastic-Agent Label for the Agent team labels May 9, 2023
@ycombinator ycombinator requested a review from cmacknz May 9, 2023 20:44
Copy link
Contributor

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Copy link
Member

@rdner rdner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree with this change, the whole point of the linter is to catch things like this.

Why is it a problem to have the linter issue reported in a PR made for development?

The linter does not block the CI or merging, this is only a notification and a reviewer decides whether to approve a PR with a linter issue or not. We should not hide issues only because they don't look nice on PRs.

@ycombinator
Copy link
Contributor Author

@rdner I'm not following what you're suggesting. The changes to the linter rules in this PR make things stricter. My intention was to have the linter complain on PRs for the following use case:

  1. I make a PR in elastic-agent-libs for some functionality I need in elastic-agent.
  2. Next, I make a PR in elastic-agent to use the functionality from elastic-agent-libs. To ensure that this PR works, I add a temporary replace directive in my elastic-agent's go.mod file, something like:
    replace github.com/elastic/elastic-agent-libs => github.com/ycombinator/elastic-agent-libs v0.0.0-20230511104653-a3c9ee8d16cb
    

With the linter changes in this PR, the replace directive will get flagged in CI. I think this is a good thing as it serves as reminder not to check in that change. It's not foolproof, yes, but it's better than nothing IMO. Can you explain why having such an automated check/reminder on the PR is a bad thing?

@rdner
Copy link
Member

rdner commented May 11, 2023

@ycombinator wait, this check was enabled before. That's why I thought you disabled it. Seems like @cmacknz removed it in #1478 not sure why, there is no explanation. Of course I'm for having this and not against, sorry, I misunderstood the change.

@ycombinator
Copy link
Contributor Author

ycombinator commented May 11, 2023

@rdner Thanks for pointing me to #1478. I will wait to get @cmacknz's opinion on this PR now, just to make sure it's not inadvertently re-introducing some problems that #1478 was trying to solve.

Copy link
Member

@cmacknz cmacknz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given I left a comment for every removed linter in https://github.com/elastic/elastic-agent/pull/1478/files except this one, I am quite sure I simply removed gomoddirectives unintentionally.

@cmacknz
Copy link
Member

cmacknz commented May 16, 2023

It is also possible I didn't read the linter configuration closely enough to see that replace-allow-list: exists to silence the warnings for replace directives we want to keep. Either way, LGTM.

@ycombinator ycombinator merged commit 3aeb206 into elastic:main May 16, 2023
@ycombinator ycombinator deleted the golangci-lint-gomod-no-replace branch May 16, 2023 13:56
mergify bot pushed a commit that referenced this pull request May 16, 2023
* Add linting rules for `replace` directives in go.mod

* Adding test replace dependencies

* Commenting out local replace

* Run go mod tidy

* Enable the linter

* Add back both test examples

* Removing local example

* Trying out nolint

* Trying again

* Trying another way

* Removing test replace directive

(cherry picked from commit 3aeb206)
ycombinator added a commit that referenced this pull request May 16, 2023
* Add linting rules for `replace` directives in go.mod

* Adding test replace dependencies

* Commenting out local replace

* Run go mod tidy

* Enable the linter

* Add back both test examples

* Removing local example

* Trying out nolint

* Trying again

* Trying another way

* Removing test replace directive

(cherry picked from commit 3aeb206)

Co-authored-by: Shaunak Kashyap <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v8.8.0 Automated backport with mergify Team:Elastic-Agent Label for the Agent team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants