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

introduce code checker and code cleanup #171

Merged
merged 16 commits into from
Aug 25, 2018

Conversation

mavimo
Copy link
Collaborator

@mavimo mavimo commented Aug 15, 2018

Hi @xetys @JohnnyQQQQ I do a bit of refactoring (I start with small change but than "degenerate" in a large amount of changes, sorry :|).
I try to have separate commit so you can check commit-by-commit instead doing a large code review (each commit is described below).

Before you start to check let me explain a bit motivation that move me to refactor the code:

  • I will make clear and "less error prone" use this library so I start to improve code with code checkers (unused, gosimple, mispell, unparam and staticcheck), each commit introduce a checker than the related fixes to make it green:

    • introduce unused code check (see ca4a535)
    • fix issue reported form unused (see 909e551)
    • introduce gosimple code checker (see 733cf88)
    • fix issue reported form gosimple (see a7bdad0)
    • introduce mispell code checker (see aa1f8ea)
    • fix issue reported form mispell (see 8578ef7)
    • add unparam code checker (see 2342234)
    • introduce staticcheck code checker (see 32c35a4)
    • fix issue reported form staticcheck (see 7c12a0b)
  • Codeclimate report some issue on duplicated code or code too complex to use, after some investigation I start to clean the code and remove duplicate:

    • remove duplicated coded reported from code climate (see a6e5e5f)
    • remove code duplicated from hetzner-go package and various clanup (see c7b8919)
  • There are some function that require "correlate invokation" (eg after we create the cloud priovider we should call init method and than set the cloud init string. This is an anti-pattern since a user do not know that we should always invoke 3 function in this sequence each time. The next step was related to factory "helper" function that generate two object from different packages, most of the time thant we ignore the second object from the codebase, so I prefer to remove helper and make clear what/when we need to create a cloud provider and cloud manager:

    • centralise creation of cloud provider using the cluster information in factory method instead of using methods to set the value after creation (see cbfc457)
    • remove helper that generate provider and manager; they are two unrelated object and we force package to be mixed; at last this method was used but manager was ignored from invocation (see f393126)
  • The last part of this refactoring is related to the "utils" we have. Most of the time utils are bad file since include a potpurri of function that do not have a well defined scope and make harder to understand it. Having public method in utils expose method from package that overcomplicate the public surface. At last a package should not panic but need to return errors and eventually command use this error to panic.

    • remove utils that are never used in codebase (see 018f84d)
    • allow generate key-pair function to return error instead of panic (see 972259b)

I hope that I not misunderstand the current codebase and the changes are good for this codebase.

PS: after this and other PRs I do today I'm planning a bit of refactoring also on addon (I think that "fail" is not a good idea, and the addon.Install() method should return and error, than we can fail in command; and command (moving to a structure like what drone/drone does since using init method should create issue to other library that try to include some package from it, maybe we should discuss it before start to develop it?

PS2: I forgot to mention that I tested (using hetzner cloud provider E2E) create a key, create a cluster, add a worker, get kubectl config, list ssh key, list ssh cluster, delete the cluster; if there are other tests that you think should be done let me know.

PS3: the last commit (see 37126dc) contains post rebase fixes

@mavimo mavimo changed the title introduce unused check that report unused variable or functions introduce unused & gosimple code check Aug 15, 2018
@mavimo mavimo force-pushed the introduce-unused-check branch from a4b40f9 to a299de8 Compare August 15, 2018 20:13
@mavimo mavimo changed the title introduce unused & gosimple code check introduce code checker and code cleanup Aug 15, 2018
@mavimo mavimo force-pushed the introduce-unused-check branch from 56bcdbd to d98e5d3 Compare August 15, 2018 21:02
@mavimo
Copy link
Collaborator Author

mavimo commented Aug 23, 2018

@xetys I'll rebase it in few hours, maybe you can start to check and indicate me if there is something wrong that I can fix after rebase :)

@xetys
Copy link
Owner

xetys commented Aug 23, 2018

I merged some of your PRs, but we have now a little conflict here 😄

@xetys
Copy link
Owner

xetys commented Aug 23, 2018

That's a huge PR and I'm sure it's all good with it. However I will spend some more time on this one before merging, at least to learn what I was doing wrong 😄

@mavimo mavimo force-pushed the introduce-unused-check branch from d98e5d3 to 5b57ac6 Compare August 23, 2018 19:12
@mavimo mavimo force-pushed the introduce-unused-check branch from 5b57ac6 to 37126dc Compare August 23, 2018 19:21
@xetys xetys merged commit add8676 into xetys:master Aug 25, 2018
@mavimo mavimo deleted the introduce-unused-check branch August 25, 2018 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants