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

Update github.com/rubenv/sql-migrate #9769

Merged
merged 1 commit into from Aug 23, 2021
Merged

Conversation

howardjohn
Copy link
Contributor

@howardjohn howardjohn commented Jun 3, 2021

What this PR does / why we need it:
Updates the dependency on github.com/rubenv/sql-migrate. This drops the total LOC imported by this library from 13.7M to 11.9M due to dropping and import on github.com/go-kit/kit.
Fixes #9770

See prometheus/common#255 for more information on why this may be important
Special notes for your reviewer:
Note: for this dependency there are no tags, so master was used. According to the author in rubenv/sql-migrate#158, there will never be a breaking change to this library, so it should be safe to update.

If applicable:

  • this PR contains documentation
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

@helm-bot helm-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 3, 2021
Copy link
Contributor

@cndoit18 cndoit18 left a comment

Choose a reason for hiding this comment

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

Hello
PRs must be linked to an existing issue. If none exists, may you please open one explaining the issue and expected outcome?
Please also write tests to test and verify this PR fixes your issue. Thanks.

@howardjohn
Copy link
Contributor Author

@cndoit18 I opened #9770 which this fixes. I am not sure its relevant to add a test, this is not a behavioral issue but rather an issue that we are importing too many things.

@mattfarina mattfarina added this to the 3.7.0 milestone Jun 4, 2021
@howardjohn
Copy link
Contributor Author

Any feedback on what is needed to get this merged?

@bacongobbler
Copy link
Member

bacongobbler commented Jun 16, 2021

Note: for this dependency there are no tags, so master was used. According to the author in rubenv/sql-migrate#158, there will never be a breaking change to this library, so it should be safe to update.

While I understand the sentiment, we have been burned by similar arguments made in the past.

We will have to check the changes made upstream to verify. As this doesn't propose any changes to Helm's behavior and it introduces a low-to-moderate level of risk, it is low in the priority queue. but it is slated for the next Helm release which is scheduled for September 8th, so it will be reviewed before that time.

Thanks for being patient.

@howardjohn
Copy link
Contributor Author

I would encourage you to reconsider the priority, as the change is quite small and the impact of this change extends beyond Helm, as you are importing a gigantic library, any consumers of helm as a library are also required to import it, and all importers of that library, and so on.

A considerable effort across the golang ecosystem has been made to fix this bloated dependency problem, but it requires buy-in from everyone.

go.mod Outdated
@@ -13,7 +13,6 @@ require (
github.com/containerd/containerd v1.5.0-rc.3
github.com/cyphar/filepath-securejoin v0.2.2
github.com/distribution/distribution/v3 v3.0.0-20210804104954-38ab4c606ee3
github.com/docker/distribution v2.7.1+incompatible
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't intentionally remove this, go mod tidy wants to remove it

Copy link
Member

@bacongobbler bacongobbler left a comment

Choose a reason for hiding this comment

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

should be good to go after a rebase.

Signed-off-by: John Howard <howardjohn@google.com>
@howardjohn
Copy link
Contributor Author

@bacongobbler thanks, should be good to go now

@bacongobbler
Copy link
Member

Sorry, forgot to mention yesterday. Due to the code size, as per policy this will need another maintainer's sign-off before we can merge.

https://github.com/helm/helm/blob/main/CONTRIBUTING.md#size-labels

@howardjohn
Copy link
Contributor Author

Sorry, forgot to mention yesterday. Due to the code size, as per policy this will need another maintainer's sign-off before we can merge.

https://github.com/helm/helm/blob/main/CONTRIBUTING.md#size-labels

Fair enough, although the doc does say ignoring generated code - the change is 2 lines of real change and the rest is generated

Copy link
Contributor

@hickeyma hickeyma left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @howardjohn

@hickeyma hickeyma merged commit 4a0b3d8 into helm:main Aug 23, 2021
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.

Drop import on github.com/go-kit/kit
6 participants