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

Add bash completion for swarm ca #268

Merged
merged 1 commit into from
Jun 30, 2017

Conversation

albers
Copy link
Collaborator

@albers albers commented Jun 29, 2017

Ref: reference page
Ping @sdurrheimer for zsh completion

Signed-off-by: Harald Albers <github@albersweb.de>
@codecov-io
Copy link

codecov-io commented Jun 29, 2017

Codecov Report

Merging #268 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #268      +/-   ##
==========================================
+ Coverage   46.84%   46.85%   +<.01%     
==========================================
  Files         172      172              
  Lines       11692    11692              
==========================================
+ Hits         5477     5478       +1     
+ Misses       5903     5902       -1     
  Partials      312      312

@dnephin
Copy link
Contributor

dnephin commented Jun 29, 2017

@albers Thanks for all your work on bash completion! Now that github has better support for looking at individual commits within a PR, what do you think about having a single PR with multiple commits? I think that might make it easier to batch review them all at once.

cc @thaJeztah

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

having multiple commits in a single PR would work yes; perhaps taking the released version into account (i.e. not a single PR for changes in a released version and upcoming version) 👍

@albers
Copy link
Collaborator Author

albers commented Jun 30, 2017

@dnephin I think it's better to keep them in individual PRs. This way, they can be more easily scheduled for different releases. If we batch them, we would get into trouble if the schedule for one of the associated feature changes, see #118 for an example.

Your proposal also does not fit well into my workflow. I usually have to do my work in very short time slots after work or in breaks. I'd like to make available the results as soon as possible. Having to sort these by proposed release dates of the associated features would force me to queue already finished work for an unknown amount of time.
(The current bunch of PRs is not typical. It was the result of taking one day off after being forced to pause my work on Docker for several weeks (repo reorg, completion not available in dev container until moby/moby#33801, vacation, a conference, high workload, family).)

Third, I found it hard to find reviewers for bigger changes in bash completion in the past. Such PRs could be hanging around several weeks. It's simply that there are not so many mainainers familiar with bash scripting. If we combine several features from e.g. swarm, builder and daemon into one PR, more reviewers would be needed.

Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐮

@vdemeester vdemeester merged commit afcc75d into docker:master Jun 30, 2017
@albers
Copy link
Collaborator Author

albers commented Jun 30, 2017

@dnephin After all, this is about coupling, which should be avoided.

@albers albers deleted the completion-swarm-ca branch June 30, 2017 09:44
@dnephin
Copy link
Contributor

dnephin commented Jun 30, 2017

Ok, if this is easier for you that's fine with me. I wouldn't want your change to sit around, but if you happen to work on something that could conflict you could push it to the same PR. As long as they are in separate commits it should be just as easy to cherry-pick for multiple releases.

For reviewing, I'm hoping that by adding more validation (and possibly even test coverage) reviews will be faster. Right now I think we have to manually test the change, which can take a while.

#266 should be a good start for validation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants