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

Importing requires too much duplicate code #98

Closed
macdabby opened this issue Mar 5, 2018 · 8 comments
Closed

Importing requires too much duplicate code #98

macdabby opened this issue Mar 5, 2018 · 8 comments

Comments

@macdabby
Copy link

macdabby commented Mar 5, 2018

I am working on a project with go migrations. As per the instructions, the entire example/main.go file needs to be copied. This can lead to outdated versions of the methods being called, even after upgrading the package itself. There are already 2 slightly different versions of the main.go file in the repo.

@macdabby
Copy link
Author

macdabby commented Mar 5, 2018

I have begun refactoring this so that it can be imported into a project by copying a very simple .go file that does not contain any logic at all. This way if the package is updated, it will operate under the updated version. There are some test issues that i'm not sure how to fix yet, but here's what I have so far: #97

@owais
Copy link

owais commented Apr 11, 2018

Thanks! I just came across the same issue and was thinking the same thing.

@VojtechVitek This looks promising. Could you please take out some time to review this if possible? :)

@macdabby
Copy link
Author

We had a PR that was vetoed. If you're interested we ended up forking this here: https://github.com/discovery-digital/goose , but I'd be happy to support merging those changes back into the master fork.

@VojtechVitek
Copy link
Collaborator

For reference: #97


Copying ~30 lines of code logic is not a big deal, is it?
https://github.com/pressly/goose/blob/master/cmd/goose/main.go#L46-L77

However, I understand the raised concern - if we broke the pkg API, then all the consumer's copy/pasted main() functions could break too.

I'm open to suggestions & improvements, however I don't want to bake unnecessary CLI logic into the pkg API itself. That's why I didn't accept #97.

Can we simplify main() and keep the API stable? Yes, totally.
Can we abstract main() completely to the pkg level? I don't think this is a good idea.

@macdabby
Copy link
Author

Copying 30 lines isn't. Maintaining 30 lines is. Like I pointed out in the PR, there are already two copies of that code in the repo that have differences where they should be identical.

Breaking those 30 lines up into a few smaller methods and including 2 lines i think would be reasonable compromise. I'm not sure I understand the full scope of the project enough to determine how to break that up.

@owais
Copy link

owais commented Apr 12, 2018

If upstream promises to never change the 30 lines then it's OK but that clearly is not the case. I don't want my custom binary to silently divert from upstream binary. I would rather have loud and explicit breakage instead of silent code deviations. The breakage problem is already solved by Dep and soon Vgo. Anyone consuming goose as a library would have to pin to a major version or risk breakage. This isn't anything new and IMO people would actually expect this to happen.

@VojtechVitek
Copy link
Collaborator

VojtechVitek commented Apr 12, 2018

I'll give you some more context: For my company, I tweaked the main.go a lot, since I'm using goose to deploy DB schema changes and Go migrations to multiple geographical regions with independent databases. The custom binary lets me switch on the runtime configuration that is specific to the microservice, database and also to the region.

I don't see much value in the example main.go file, outside of showing other developers what they can do with this fork. That's why open-sourced it in the first place.

What I do care about is that the API of github.com/pressly/goose package is and will be stable, so we don't have any regressions. If we ever intend to break the API, we'll have to bump the major version, so the tools like vgo or dep don't it pick up blindly.

@VojtechVitek
Copy link
Collaborator

VojtechVitek commented Apr 12, 2018

If you care to update the example main.go and keep it in sync with cmd/goose/main.go, please create a PR. I welcome any help from the community.

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

No branches or pull requests

3 participants