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

v1 bug: Not everyone is using modules, mind what you push to master #947

Closed
l0k18 opened this issue Nov 25, 2019 · 3 comments
Closed

v1 bug: Not everyone is using modules, mind what you push to master #947

l0k18 opened this issue Nov 25, 2019 · 3 comments
Assignees
Labels
area/v1 relates to / is being considered for v1 kind/bug describes or fixes a bug status/triage maintainers still need to look into this

Comments

@l0k18
Copy link

l0k18 commented Nov 25, 2019

I have forked this repo because of this. Altsrc was buggy and I ended up making a manual reader of env/args inputs anyway and the latest or second latest push to master has broken a heap of my stuff relating to the cli.StringFlag and cli.StringSlice, the latter in particular, which is throwing errors over my use of the (original) type for the second one in particular which clearly has been turned into a struct hence rejecting []string types. I fixed it first by checking out a new branch on the v1/v1.22 and sure enough my code compiles and then I forked it and put that commit at master. I sincerely hope you will do the same, it isn't a big ask to NOT push breaking code to master when 2 is not yet a 2.0

Please, please, for the devs building this, don't assume everyone is migrating to modules in a hurry. They are unstable and have changed 3 times in the last 6 months already how they work, and complicate my workflow a lot.

I encountered this issue as I tried to build my project on a fresh clean base, on ubuntu (and in Termux on my android device) and suddenly this import (and lightninglabs gozmq also did the same thing in recent weeks, I just migrated to upstream) was throwing errors over []string not being commutable with cli.StringSlice

For anyone who has just added this repo to their project for the first time, that Master is going to be pinned in the go.mod also, so many people will run up against this and have to learn the hard way how to change the pin to a stable version. Unless version 2 is release grade now it should be considered unstable and experimental and certainly should not be pinned to the master HEAD.

@l0k18 l0k18 added status/triage maintainers still need to look into this kind/bug describes or fixes a bug area/v1 relates to / is being considered for v1 labels Nov 25, 2019
@coilysiren
Copy link
Member

Few things:

  • v2 is in fact at 2.0 now
  • v2 essentially does not fully support altsrc, this is a compromise we made in order to push v2 out this year
  • v3 will remove altsrc (the package, not the functionality) entirely, so there will likely be no v2 release that includes a stable and production grade altsrc package

@AudriusButkevicius
Copy link
Contributor

Sorry, but life goes on, and people want to make breaking changes and evolve the project.

The fact that you do not use ANY form of vendoring, even gvt or something even more ancient is your own problem. You want to live on the edge (master), so be it, but deal with the consequences don't be surprised when people break you.

And we will do it again, but I guess you already forked, so it shouldn't matter too much to you.

@l0k18
Copy link
Author

l0k18 commented Nov 26, 2019

sure, I can understand that. I wasn't aware you had pushed the 2.0 forward in the time between me picking this as the most usable CLI args/env/config library existing. I had ideas about how to improve it anyway so I'll slate that for certain into my future work.

Thanks for the great work, btw. This is definitely the most approachable and robust library for the job that I know of. The workarounds I spent a lot of the last 6 months making for what it didn't implement has pretty well circumscribed exactly what I need from it anyway.

I already know, roughly, what is needed to provide the equivalent of the altsrc library's functionality. In my code I ended up having to keep 4 separate source files in sync with the urfave/cli configuration. My solution would be to integrate those external parts and generalise them. One is an empties declaration, that makes all the re-directable pointers available and eliminates nil panics, another is the code that reads in the env/args and overrides the configuration which is, part 4, marshalled from the original configuration file (in the app I have built from). I just didn't integrate them sooner because adding and removing configuration items doesn't take long enough to be annoying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/v1 relates to / is being considered for v1 kind/bug describes or fixes a bug status/triage maintainers still need to look into this
Projects
None yet
Development

No branches or pull requests

3 participants