Skip to content
This repository has been archived by the owner on Oct 10, 2023. It is now read-only.

Reverting tools dependency to v0.1.10 #3433

Merged
merged 6 commits into from
Sep 26, 2022

Conversation

saji-pivotal
Copy link
Contributor

@saji-pivotal saji-pivotal commented Sep 20, 2022

What this PR does / why we need it

This PR attempts to fix linters getting short circuited because of an incompatible dependency version
This PR also fixes (or skips) linter issues in the following directories

cli/core
cli/runtime
cmd/cli/plugin/cluster

Which issue(s) this PR fixes

Fixes #3438

Describe testing done for PR

  1. Ran the golangci-lint binary wherever possible locally
  2. This PR relies on the linter checks that get run as part of the CI

Release note

Reverted the tools dependency to v0.1.10
Fixed or skipped linter issues

Additional information

Special notes for your reviewer

@codecov
Copy link

codecov bot commented Sep 20, 2022

Codecov Report

Merging #3433 (33d7dbe) into main (69c6e92) will decrease coverage by 0.26%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #3433      +/-   ##
==========================================
- Coverage   53.99%   53.72%   -0.27%     
==========================================
  Files          91       91              
  Lines       10006    10006              
==========================================
- Hits         5403     5376      -27     
- Misses       4169     4190      +21     
- Partials      434      440       +6     
Impacted Files Coverage Δ
cmd/cli/plugin/cluster/available_upgrade.go 16.32% <0.00%> (ø)
...in/cluster/get_machinehealthcheck_control_plane.go 11.11% <ø> (ø)
.../cli/plugin/cluster/get_machinehealthcheck_node.go 9.30% <ø> (ø)
addons/controllers/clusterbootstrap_controller.go 62.79% <0.00%> (-2.31%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@vuil
Copy link
Contributor

vuil commented Sep 20, 2022

To see the full extent of the linter warnings, try applying this change to the toplevel Makefile temporarily to your PR:

-               $(GOLANGCI_LINT) run -v --timeout=10m || exit 1; \
+               $(GOLANGCI_LINT) run -v --timeout=10m; \

@saji-pivotal saji-pivotal marked this pull request as ready for review September 21, 2022 00:21
@saji-pivotal saji-pivotal requested a review from a team as a code owner September 21, 2022 00:21
@saji-pivotal saji-pivotal changed the title WIP: DO NOT MERGE: Reverting tools dependency to v0.1.10 Reverting tools dependency to v0.1.10 Sep 21, 2022
@saji-pivotal saji-pivotal force-pushed the fix-linters branch 3 times, most recently from e91bcf6 to 3ea9239 Compare September 21, 2022 19:10
@saji-pivotal
Copy link
Contributor Author

@anujc25 I'm not sure on a per-file level how many linter errors we're skipping
I have filed the following issues to refactor the code, and actually fix the linter issues we're seeing

#3470
#3471

@anujc25
Copy link
Contributor

anujc25 commented Sep 22, 2022

Sounds good. This PR looks to me.

Copy link
Contributor

@anujc25 anujc25 left a comment

Choose a reason for hiding this comment

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

LGTM

@anujc25 anujc25 added the ok-to-merge PRs should be labelled with this before merging label Sep 23, 2022
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
cla-not-required ok-to-merge PRs should be labelled with this before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix linter issues in the cli modules
4 participants