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

Remove excessive dependencies on go-kit #8711

Closed
wants to merge 1 commit into from

Conversation

howardjohn
Copy link
Contributor

What this PR does / why we need it:

This downgrades our version of sql-migrate. This results in dropping a ton of dependencies - prometheus/common#255 (comment) has some more info

Essentially a partial revert of 148d94b

@helm-bot helm-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 10, 2020
Signed-off-by: John Howard <howardjohn@google.com>
@bacongobbler
Copy link
Member

bacongobbler commented Sep 10, 2020

@howardjohn can you please describe the issue you are trying to solve here? Based on the discussion in prometheus/common#255 (comment) this does not appear to be an issue, so I'm curious why you're bringing this up.

What concerns me more than lines of code is a code regression due to a version downgrade.

@howardjohn
Copy link
Contributor Author

The primary concern is we updated our helm version and it brought in a substantial amount of dependencies. I find it important to minimize dependencies when possible to avoid bloat and reduce module conflict pains.

@bacongobbler
Copy link
Member

bacongobbler commented Sep 10, 2020

On master:

><> cloc vendor
    4015 text files.
    3826 unique files.                                          
     475 files ignored.

3 errors:
Line count, exceeded timeout:  vendor/golang.org/x/net/idna/tables10.0.0.go
Line count, exceeded timeout:  vendor/golang.org/x/net/idna/tables11.0.0.go
Line count, exceeded timeout:  vendor/golang.org/x/net/idna/tables9.0.0.go

github.com/AlDanial/cloc v 1.82  T=17.21 s (206.4 files/s, 65709.3 lines/s)
--------------------------------------------------------------------------------
Language                      files          blank        comment           code
--------------------------------------------------------------------------------
Go                             3156          94947         125477         863473
Markdown                        152           4466              0          12646
Protocol Buffers                 66           3653          11943           6164
Assembly                         39            314            380           1741
JSON                              5              1              0           1672
YAML                             84            208             39           1255
Bourne Shell                     18            123            341            754
Bourne Again Shell                3             45             57            401
make                             18            106            119            371
Dockerfile                        2             16              9             55
TOML                              4             17             78             50
Starlark                          2              4             37             36
C                                 1              8              7             24
PowerShell                        1              3              8              1
vim script                        1              0              0              1
--------------------------------------------------------------------------------
SUM:                           3552         103911         138495         888644
--------------------------------------------------------------------------------

Compared to this PR:

~/code/go/src/helm.sh/helm ><> cloc vendor
    4001 text files.
    3813 unique files.                                          
     472 files ignored.

3 errors:
Line count, exceeded timeout:  vendor/golang.org/x/net/idna/tables10.0.0.go
Line count, exceeded timeout:  vendor/golang.org/x/net/idna/tables11.0.0.go
Line count, exceeded timeout:  vendor/golang.org/x/net/idna/tables9.0.0.go

github.com/AlDanial/cloc v 1.82  T=17.28 s (204.9 files/s, 65086.2 lines/s)
--------------------------------------------------------------------------------
Language                      files          blank        comment           code
--------------------------------------------------------------------------------
Go                             3145          94492         125099         858074
Markdown                        152           4393              0          12443
Protocol Buffers                 66           3653          11943           6164
JSON                              5              1              0           1672
Assembly                         37            273            332           1609
YAML                             84            211             40           1270
Bourne Shell                     18            123            337            750
Bourne Again Shell                3             45             57            401
make                             18            106            119            371
TOML                              5             25             81             75
Dockerfile                        2             16              9             55
Starlark                          2              4             37             36
C                                 1              8              7             24
PowerShell                        1              3              8              1
vim script                        1              0              0              1
--------------------------------------------------------------------------------
SUM:                           3540         103353         138069         882946
--------------------------------------------------------------------------------

Net result still looks the same to me. We're looking at a difference of about 6k LOC overall by removing 14 files. That seems like a stark difference compared to the claimed 5.5M+ lines of code supposedly removed in prometheus/common#255.

Have you tested this yourself? How should I be testing this?

I find it important to minimize dependencies when possible to avoid bloat and reduce module conflict pains.

While I understand that, downgrading a dependency seems like a backwards way to reduce bloat. Have you tested to see how these changes may impact Helm itself?

@howardjohn
Copy link
Contributor Author

Have you tested to see how these changes may impact Helm itself?

I have ran the tests and It seems you have CI set up, is there other testing developers are expected to do?

Have you tested this yourself? How should I be testing this?

I have tested this myself by counting the lines of codes downloaded by go modules when importing the repo rather than the vendor directory (keep in mind many users do not use vendor). You can also see from the go.sum a large amount of dependencies that are no longer present wth this PR

@bacongobbler
Copy link
Member

bacongobbler commented Sep 15, 2020

I would be amenable to an upgrade of sql-migrate that removed the code bloat. But, considering that there is a greater risk of a regression being introduced than a few extra lines of code being downloaded during a go build (an inconvenience, but doesn't introduce any significant harm), I'm going to close this as something we're not going to merge.

Thank you for your input.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants