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

proposals to revive pflag as an actively maintained repo #386

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fredbi
Copy link
Collaborator

@fredbi fredbi commented Oct 2, 2023

Signed-off-by: Frédéric BIDON <fredbi@yahoo.com>
@fredbi
Copy link
Collaborator Author

fredbi commented Oct 2, 2023

cc: @therealmitchconnors


### go version

This repository supports _at least_ the 2 latest stable versions of the language.

Choose a reason for hiding this comment

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

Are there other major golang projects which act as dependencies to the ecosystem at large which have this sort of stability guarantee? Or are they wider than this?

Of note, Golang 1.20 was only released a year ago, which seems like a comparatively small window.

Copy link

Choose a reason for hiding this comment

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

Given that Go itself only supports the two most recent major releases with security updates, this seems appropriate to me.

ref: https://go.dev/doc/security/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My suggestion is that we commit to 2 releases (i.e. run CI on them). that doesn't mean necessarily that we ENFORCE people to upgrade (go.mod require version may lag behind)

Comment on lines +104 to +106
> TODO: is this something that is still needed? Anyhow, check if the CLA terms are up to date.
> The Linux Foundation has introduced [EasyCLA](https://docs.linuxfoundation.org/lfx/easycla).
> I believe that `cla.assistant.io` is a bit old.

Choose a reason for hiding this comment

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

I'm not a huge fan of this. I see often that folks will make a quick PR and then not follow up with the CLA (due to time or whatever) and then the PR just waits indefinitely.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@josegonzalez I am inclined to agree with the CLA being more or less just boilerplate.
However: I'd like to remain consistent with the other spf13 repos. viper has CLA check, cobra has CLA check, so should pflag.

Choose a reason for hiding this comment

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

Thats fair.

Comment on lines +112 to +114
> TODO: this requirement is debatable.
> On the one hand, it is certainly something many junior developers are perplex about.
> On the other hand, supporting so many major pieces of infrastructure is a big responsibility.

Choose a reason for hiding this comment

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

I will very often make a PR via the Github UI - for doc changes or minor cosmetic issues. I think its possible to sign in the UI, but I very frequently forget to, and then its a pain to come back and fix it locally (sometimes to the point of me just closing the PR and saying someone else can re-contribute it).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@josegonzalez this is simple sign-off (git commit -s, with email) not GPG signature.

Comment on lines +63 to +65
> TODO: agree on the initial .golangci.yml setup, with a no-nonsense approach to linting. Code quality without nitpicking... :)
> As a started, here is a sample configuration that I use on a daily basis for personal projects (no strong agenda on this):
> [golangci-lint.yml](https://github.com/fredbi/gflag/blob/master/.golangci.yml).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Linters enabled in Viper: https://github.com/spf13/viper/blob/master/.golangci.yaml

Though I'm not sure all of them is necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sagikazarmark yeah I am going to align with the ones (also comparing with cobra)

Comment on lines +136 to +137
> I am proposing a new git repo as I found rather impractical to maintain a nested `go.mod` in the main repo.
> Further, we most likely want to version contributed features independantly.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Contrib repos are generally hard to maintain, especially as they grow over time.

I'd probably start with a list of extensions in a markdown file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sagikazarmark point taken. Let's start with a mere document with links

3 to 5 volunteers. No less, no more.

Would be nice to open a slack or discord channel to make debate more lively.
Otherwise, we may use github discussions.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Otherwise, we may use github discussions.
Otherwise, we may use GitHub discussions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Both makes sense. We can have a channel on Gophers Slack. GH Discussions is better for archiving.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok let's try both


Proposed principles:

* promise of compatibility (i.e. no breaking `v2` ever)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need to make this promise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sagikazarmark ok. Still, I'll refrain as much as I can from having to issue a breaking V2

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a good thumb rule, but as soon as you promise it, it's hard to take it back.

* post a comment/reply to the OP

Proposed issue labels:
* bug
Copy link
Collaborator

Choose a reason for hiding this comment

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

I usually like prefixing labels with things like kind/, resolution/, etc. It makes distinguishing between different labeling schemes easier.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok good point

@@ -0,0 +1,104 @@
# `pflag` roadmap
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think most of this document could become issues in the repo. Easier to track progress.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep. Exactly. Now removing that section and replacing it with issues

@josegonzalez
Copy link

@fredbi what do we need to do to move forward here?

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.

None yet

4 participants