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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fall release 馃崄 - 2021 #1496

Closed
umarcor opened this issue Sep 24, 2021 · 36 comments
Closed

Fall release 馃崄 - 2021 #1496

umarcor opened this issue Sep 24, 2021 · 36 comments
Assignees

Comments

@umarcor
Copy link
Contributor

umarcor commented Sep 24, 2021

Last release was planned for May 2021 and released in July. Next week, it will be 3 months since that. In this period, 3 PRs were merged.

Is there any plan for merging PRs and releasing a new version in the following weeks?

Release candidate (please try out and test!)

To Do

Approved by a maintainer but merge seems to have been forgotten:

Left pending in the previous release (#1388):

Small completion fixes from the community:

Worth considering:

Some random cool ones:

Done

Delayed to a later release

@marckhouzam
Copy link
Collaborator

marckhouzam commented Sep 25, 2021

To help the maintainers how about we try to weed through some of the PRs?
Here's what I came up with.

My apologies to the community for not listing some of the ones I'm not familiar with, it does not mean they are not just as valuable, but I had to start somewhere.

Adding projects to projects_using_cobra.md:

@umarcor has been efficient and rebased 6 contributions into one PR to avoid the merge conflicts: #1454
This will allow to close: #987 #1371 #1380 #1407 #1446 #1451

Approved by a maintainer but merge seems to have been forgotten:

#1161 DisableFlagParsing must trigger custom completion for flag names
#841 Generalize ValidArgs: use it implicitly with any validator

Small completion fixes from the community:

#1455 Completion: Capitalize short desc and remove extra space from long
#1321 fix: unbound variables in bash completion
#1450 feat(completion): add default completion hidden if there are no subcmds - not ready
#1372 Update shell_completions.md to have automatically rootcmd
#1467 feat: make InitDefaultCompletionCmd public
#1473 completions: fix mixed tab/spaces indentation
#1459 fix: typo in {bash,zsh}_completions.go
#1444 fix: improve completions help formatting
#1495 Remove indentation causing RST errors

Some random cool ones:

#1231 add colors to CLI commands
#1003 Add groups for commands in help

It would be nice to continue the discussion about how to avoid bringing in all the Viper dependencies.

There are some nice documentation cleanup and CI improvements from @umarcor.

@umarcor
Copy link
Contributor Author

umarcor commented Sep 25, 2021

@marckhouzam, a hint: make a list (add - before #). That will make the references be rendered as below.

@umarcor has been efficient and rebased 6 contributions into one PR to avoid the merge conflicts: #1454
This will allow to close: #987 #1371 #1380 #1407 #1446 #1451

Should there be any similar PR which I missed, please let me know and I will include it.

Approved by a maintainer but merge seems to have been forgotten:

#841 Generalize ValidArgs: use it implicitly with any validator

#841 is now based on #1426, which should be merged first.
In my initial #841 proposal (back in March 2019), reworking the tests was not in the scope.
It was done upon maintainers requests, but it then seemed to increase the complexity so that no one would actually review and merge. Hence, a few months ago I split #1426 in the hope that it will be easier to review.

Note that #841 is, in fact, a fix for an incomplete implementation. It is not a new feature per se. Moreover, backwards compatibility is preserved. I am honestly frustrated with that one because it is ready to be merged since 1-2y ago. During the last months, I refrained myself from working on improving the docs/website, because I'm not confident about the efforts being honored. It's nothing personal, tho; just the particularities of this project (#959). I have the perception that there is a strong pushback to any minimally meaningful change, because of how many big players depend on this project. My take on it is that, if those big players care, they should get people involved in the development to ensure that we catch regressions. IMHO, gatekeeping is not acceptable.

It would be nice to continue the discussion about how to avoid bringing in all the Viper dependencies.

I'm personally good with keeping that out of the discussion for now.
Although I do think that a reorganisation is required, that will be necessarilly very disruptive.
Fixing that requires changing the import path of the lib, the cli, or both of them.
That's something we want to be careful about, in the sense that we want to advertise it very loudly months before actually doing it.
Therefore, we need to first have some rythm in the development (complying with the quarterly release statement in https://github.com/spf13/cobra/blob/master/CONDUCT.md). Then, we need to update the site so that we have a "news" section for communicating meaningful changes. Hugo has RSS support, so we'd get that for free. Last, we can afford going forward with disruption.
See #1240.


Some PRs which were left pending in the previous release (#1388):

Chores:

Documentation:

Tests:


Worth considering:


Some long standing PRs:


It's a duplicate, can be closed:

@marckhouzam
Copy link
Collaborator

@umarcor nice trick to automatically include the title of the PR and its state so easily!

How do you feel about making a unified "proposed" list in the description of this PR? That way it can keep being updated and the maintainers don't have to sift through the comments.

@umarcor
Copy link
Contributor Author

umarcor commented Sep 25, 2021

@marckhouzam I copied all the refs to the description (first message) above. Let me know if you (or anyone else) want to add/remove anything.

@jpmcb
Copy link
Collaborator

jpmcb commented Sep 27, 2021

Hi all - very busy season for everyone since Kubecon is around the corner. Let's shoot for an end of October release since we had a summer release. Thanks all for your patience and dedication to this project!!! 鉂わ笍

Pinning this as our release tracker

@jpmcb jpmcb pinned this issue Sep 27, 2021
@jpmcb jpmcb changed the title Next release? Fall release - 2021 Sep 27, 2021
@marckhouzam
Copy link
Collaborator

@umarcor would you mind adding #1509 to the list of PRs to include in the release? It is a bug fix for bash completion v2. Thanks!

@umarcor
Copy link
Contributor Author

umarcor commented Oct 19, 2021

@marckhouzam, done!

@georgettica
Copy link
Contributor

I saw the reference and wanted to know how can I help to promote my PR/others
let me know

@umarcor
Copy link
Contributor Author

umarcor commented Oct 21, 2021

@georgettica, #1372 is listed in the first message above already. Which other PRs are ready to be merged (and approved by someone) but not listed yet?

Neither @marckhouzam nor myself are allowed to merge PRs. Hence, listing them here is our best effort to help the maintainers. Nonetheless, we depend on them for moving the project forward.

@jpmcb mentioned the end of october, so I expect merging activity during the next 10 days. /cc @jharshman @spf13

@georgettica
Copy link
Contributor

@georgettica, #1372 is listed in the first message above already. Which other PRs are ready to be merged (and approved by someone) but not listed yet?
Non come to mind, but I thought maybe there is something we can do to help and validate solutions

@marckhouzam
Copy link
Collaborator

@umarcor #1510 would be nice to add to the list as well. Thanks

@umarcor
Copy link
Contributor Author

umarcor commented Oct 22, 2021

@marckhouzam, done!

@marckhouzam
Copy link
Collaborator

Another tiny one @umarcor please: #1520

@umarcor
Copy link
Contributor Author

umarcor commented Oct 27, 2021

@marckhouzam, done!

@jpmcb
Copy link
Collaborator

jpmcb commented Nov 2, 2021

This would be nice to have if possible: #1507

Gonna start slowly working through some of the pull requests

@umarcor
Copy link
Contributor Author

umarcor commented Nov 2, 2021

@jpmcb, so far we listed PRs only, not issues. Nonetheless, I believe you can edit my first message above, so feel free to add it to the list as soon as a PR is proposed.

Gonna start slowly working through some of the pull requests

I suggest you pick the "Chores" and "Documentation" first. Those should all be pretty straightforward. Then, maybe go with "Small completion fixes from the community". I believe that'll allow to get most of the work done quickly (without many conflicts).

@jpmcb jpmcb changed the title Fall release - 2021 Fall release 馃崄 - 2021 Nov 2, 2021
@yuvalsosman
Copy link

Hey,
Someone can please provide an update regarding this great feature:
Add groups for commands in help #1003 ?

@umarcor
Copy link
Contributor Author

umarcor commented Nov 4, 2021

@yuvalsosman #1003 is listed above already.

@YuviGold
Copy link
Contributor

YuviGold commented Nov 5, 2021

Hey
I would really appreciate if this one could go in as well #1500 .
It can really improve a user's usage experience with nested subcommands level completions :)

@umarcor
Copy link
Contributor Author

umarcor commented Nov 8, 2021

@YuviGold, now listed.

@marckhouzam
Copy link
Collaborator

@umarcor could you add #1541 to the release list? It is a simple fix to allow hiding the completion command as requested by @jpmcb. Thank you!

@umarcor
Copy link
Contributor Author

umarcor commented Nov 24, 2021

@marckhouzam done! I added it to #1525 as well.

@marckhouzam
Copy link
Collaborator

@YuviGold @umarcor Please be aware that I've just expressed concerns about backwards-compatibility in #1500. To be safe, I would suggest pulling it out of the release until the concerns are proven wrong.

@umarcor
Copy link
Contributor Author

umarcor commented Nov 26, 2021

@marckhouzam I believe that #841 (which should be one of the next PRs to be merged) will help decide the outcome of #1500. In #841 it is explicitly stated that a nil Args defaults to ArbitraryArgs, unlike the currently undefined/undocumented behaviour. Very precisely: fcfd90e.

@marians
Copy link
Contributor

marians commented Nov 30, 2021

Any chance to get #1003 (flag command grouping) into the release?

EDIT: I had a misconception regarding what #1003 does. Still would be nice.

@umarcor
Copy link
Contributor Author

umarcor commented Nov 30, 2021

@marians #1003 is shown in the first message already (#1496 (comment)). It was added to the list the 25 Sep (#1496 (comment)), and a user asked about it the 11 Oct (#1003 (comment)) and 4 Nov (#1496 (comment)).

@marckhouzam I added section "Delayed to a later release" above, and I moved #1500 there.

@jpmcb jpmcb self-assigned this Dec 8, 2021
@jpmcb
Copy link
Collaborator

jpmcb commented Dec 8, 2021

This release is really really close and I'm thinking I'll cut it this afternoon or tomorrow.

Please feel free to give the HEAD of the main branch a try since that's what will be cut (or the RC as it's updated).

I've marked a number of issues / PRs as milestone 2.0.0 since some are still getting community feedback, have breaking changes, or would require more work to be complete.

As always, thank you everyone for your contributions, hard work, patience, and diligence! 馃悕

@umarcor
Copy link
Contributor Author

umarcor commented Dec 8, 2021

@jpmcb, #841 was approved almost 3 years ago. I beg you to please do not delay it another 6 months.

@jpmcb
Copy link
Collaborator

jpmcb commented Dec 8, 2021

@umarcor True! Looks to have had several rounds of review and was approved by Josh but not merged. I wasn't involved in that PR but on a quick look, it appears to change several public methods which means it needs to be slated for a major version bump, not a minor version release.

My main goal with Cobra is to not unexpectedly break major downstream projects like Kubernetes or Helm. Generally, in the past, we have not done a great job of that.

Further, I have a limited amount of time to review, merge, validate, etc. It's already December and this was intended to be a Fall release and I'd say this release has already expanded it's scope beyond what we should have included in it. Big releases make for more work for the maintainers. And since I've somehow become the only maintainer, it becomes alot of work for me. So in short, please be patient.

@umarcor
Copy link
Contributor Author

umarcor commented Dec 8, 2021

@jpmcb

on a quick look, it appears to change several public methods which means it needs to be slated for a major version bump

I'm afraid that's a misunderstanding that several non-primary maintainers carryied for the last years. As far as I understand, it does not involve API changes (it's an enhancement, not removal or modification). Precisely, several public methods are maintained as aliases for backwards compatibility (as explicitly requested by Josh): https://github.com/spf13/cobra/pull/841/files#diff-d3989c40d6d1caecf765e82f992c4bdf093c103c074003e4d7f29cee6d9437a1R111-R124.

My main goal with Cobra is to not unexpectedly break major downstream projects like Kubernetes or Helm. Generally, in the past, we have not done a great job of that.

That's why #1525 was around for over a month. It was explicitly tested on Helm already, as reported by the maintainer in #1525 (comment). I'm not aware of Kubernetes developers participating in this repo.

Further, I have a limited amount of time to review, merge, validate, etc. It's already December and this was intended to be a Fall release and I'd say this release has already expanded it's scope beyond what we should have included in it. Big releases make for more work for the maintainers. And since I've somehow become the only maintainer, it becomes alot of work for me. So in short, please be patient.

I can and will be patient. However, ignoring #841 is producing triplication of features in the codebase, and conflicts between those three implementations. The more features we add without fixing incomplete implementations, the worse the codebase will be to maintain.

@jpmcb
Copy link
Collaborator

jpmcb commented Dec 8, 2021

I completely understand the frustration.

You, and many others, have worked tirelessly to maintain and improve this library. Often without feedback from the maintainers or owners of this project. And unfortunately, I've had to take on more responsibility in this library than I really have time for.

In short, I just don't have great context on that particular PR and we need to get a release pushed. Otherwise, we'll just keep delaying getting it out and, I fear, inevitably create a release that is unmanageable to consumers of Cobra.

Let's meet somewhere in the middle and get this into the next release, merged quickly after we cut v1.3.0?

@marckhouzam
Copy link
Collaborator

marckhouzam commented Dec 9, 2021

@jpmcb I've rerun the cobra-completion-testing tests as well as both Helm's test suites with the latest master branch of Cobra and things look good.

I've only done some basic manual testing with helm and kubectl but nothing bad jumped out at me.

Thanks for getting all these PRs merged!
馃殌

@georgettica
Copy link
Contributor

can you TAL at #1556?
found this when checking my PR

@jpmcb
Copy link
Collaborator

jpmcb commented Dec 9, 2021

Anyone want to take a look at #1557 quick?

@jpmcb
Copy link
Collaborator

jpmcb commented Dec 14, 2021

v1.3.0 has been cut! Thank you all for your support in getting this release out! https://github.com/spf13/cobra/releases/tag/v1.3.0

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

7 participants