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

ctlv3: fix line parsing for Windows #6385

Closed
wants to merge 7 commits into from
Closed

ctlv3: fix line parsing for Windows #6385

wants to merge 7 commits into from

Conversation

sinsharat
Copy link
Contributor

Fixed : Not able to verify transaction example via CMD
#6377

xiang90 and others added 2 commits September 7, 2016 11:21
Restore should create a snasphot. So the new db file
can be sent to newly joined member.
@sinsharat
Copy link
Contributor Author

@gyuho Can you suggest why the go get is failing. Do i need to modify something else?
thanks.

@gyuho
Copy link
Contributor

gyuho commented Sep 8, 2016

@sinsharat https://travis-ci.org/coreos/etcd/jobs/158431431#L507 says it has unused packages.

You can simply run goimports -w *.go.

And also could you change your commit title to something like ctlv3: fix line parsing for Windows?

Thanks!

@sinsharat sinsharat changed the title etcdctl v3: Not able to verify transaction example via CMD fixed ctlv3: fix line parsing for Windows Sep 8, 2016
@sinsharat
Copy link
Contributor Author

sinsharat commented Sep 8, 2016

@gyuho changed the commit title.
Just want to know where exactly you want me to run ''goimports -w *.go.''
Thanks.

@sinsharat
Copy link
Contributor Author

@gyuho ok i got the issue one of the files has unused import. Will fix it.

Thanks!

@heyitsanthony
Copy link
Contributor

This is not a good fix; there's too much code duplication and can be handled portably. Please use strings.TrimSpace.

if err != nil {
ExitWithError(ExitInvalidInput, err)
}
if len(line) == 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

line = strings.TrimSpace(line)
if len(line) == 0 {
    break
}

@sinsharat
Copy link
Contributor Author

@heyitsanthony sure will try it out.
Thanks!

@@ -0,0 +1,67 @@
// Copyright 2015 The etcd Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this file?

@heyitsanthony
Copy link
Contributor

lgtm after fixing minor nits. Thanks!

@sinsharat
Copy link
Contributor Author

@heyitsanthony plz check if its fine then will squash the commits.
Thanks!

Grow the inflights buffer as needed instead of preallocating it to its
max size. This avoids preallocating a lot of unnecessary
space (8*MaxInflightMsgs) when using lots of raft groups while still
allowing for a reasonable MaxInflightMsgs configuration.

raft: add tests for IsLocalMsg (#6357)

* raft: add tests for IsLocalMsg

* report index of failed tests

corrected the package name in logger

etcdctlv3: Readme.md updated

1. Under PUT example the put command was mentioned in capital which will
give the below error:
Error: unknown command "PUT" for "etcdctl"
Hence corrected the same.
2. The lease id is mentioned with 0x to denote hex but since its an
example, copy pasting the command will give the below error:
Error: bad lease ID (strconv.ParseInt: parsing "0x1234abcd": invalid
syntax), expecting ID in Hex
Hence modified the same to a sample correct value so that a user new to
etcd does not get confused.
3. The command ./etcdctl range foo does not work and gives the below
error:
Error: unknown command "range" for "etcdctl"
Hence corrected the same

clientv3: drain buffered WatchResponses before resuming

Otherwise, the watcherStream can receive WatchResponses in the
middle of a resume, corrupting the stream.

Fixes #6364

etcdctl: Corrected command in Readme.md (#6376)

Corrected command in Readme.md

grpcproxy: handle overloaded stream

grpcproxy: fix a data race

clientv3: use correct context in toErr (lease)

clientv3: drain buffered WatchResponses before resuming

Otherwise, the watcherStream can receive WatchResponses in the
middle of a resume, corrupting the stream.

Fixes #6364

etcdctl: Corrected command in Readme.md (#6376)

Corrected command in Readme.md

grpcproxy: handle overloaded stream

grpcproxy: fix a data race

clientv3: use correct context in toErr (lease)

etcdctl v3: Not able to verify transaction example via CMD fixed

Fixed : Not able to verify transaction example via CMD

ctlv3: fix line parsing for Windows

ctlv3: fix line parsing for Windows

ctlv3: fix line parsing for Windows

ctlv3: fix line parsing for Windows
@heyitsanthony
Copy link
Contributor

@sinsharat patch looks OK aside from the stray commits which need to be removed/squashed. Thanks!

To avoid stray commits in the future, the workflow is to use feature branches for PRs and a remote upstream to sync/rebase against master (https://help.github.com/articles/configuring-a-remote-for-a-fork/):

git checkout master
git checkout -b my-feature-branch
<do some work>
git commit
<submit PR using my-feature-branch>
<master changed? fetch from upstream and rebase feature branch>
git checkout master
git fetch upstream/master
git merge master
git checkout my-feature-branch
git rebase -i master
git push -f origin my-feature-branch

…r-from-follower

raft: add test case for leader transfer from follower
@heyitsanthony
Copy link
Contributor

@sinsharat please fix your patch so that the diff only has your changes and this should be good to merge. Thanks!

@sinsharat
Copy link
Contributor Author

@heyitsanthony i was struggling with squashing commits for a long time. somehow today i managed to do it. but now i am seeing even files changed by others have come in this place.
What do you suggest me to do about this.
Thanks for help!

@sinsharat
Copy link
Contributor Author

infact my changes not showing in this commit now.

@heyitsanthony
Copy link
Contributor

@sinsharat the suggested workflow is above. Master isn't really meant to be rebased...

Easiest way to keep using master for this PR is copy your changes somewhere, git reset --hard HEAD~1 to get back to the mainline git history, copy the changes back, add and commit, then git push -f.

@xiang90
Copy link
Contributor

xiang90 commented Sep 8, 2016

// reset back to a good historical point
git reset --hard HEAD~10
// pull down head of master
git pull original master
// apply your patch onto the head
...
// commit
git commit -am "ctlv3: fix line parsing for Windows"
// force push
git push your_remote_branch your_local_branch -f

@sinsharat
Copy link
Contributor Author

@xiang90 i did one mistake, i was not working on branch, but directly commiting to master, which is origin in my case. So for the last step what can i do.
I have been able to do all the steps except for the last one. Just not able to know what exactly should i do in my last step.

Thanks for help.

@xiang90
Copy link
Contributor

xiang90 commented Sep 8, 2016

There are still 6 commits (including the right one) and one merge. We still need to fix this.

Let me provide you a more detailed git instruction

git checkout master
git reset --hard HEAD~10
git pull original master
git checkout fix_ctl_win
// apply your code change now
// save your change
git status
// now you can see your changes
git commit -am "ctlv3: fix line parsing for Windows"
git push sinsharat fix_ctl_win

now create a new pull request on sinsharat/fix_ctl_win.

Your current pr is on your own master branch. That caused a lot of troubles. Probably we have to replace this pr by the one on your feature branch as i suggested this time.

@sinsharat
Copy link
Contributor Author

@xiang90 couple of doubts: original means coreos/etcd or sinsharat/etcd branch.
also
git checkout fix_ctl_win giving error: error: pathspec 'fix_ctl_win' did not match any file(s) known to git.

@xiang90
Copy link
Contributor

xiang90 commented Sep 8, 2016

original -> coreos/etcd

git checkout fix_ctl_win -> git checkout -b fix_ctl_win

@xiang90
Copy link
Contributor

xiang90 commented Sep 8, 2016

usually you will treat the coreos/etcd as original

you add your branch as remote

git remote add sinsharat sinsharat/etcd

then you can do git push sinsharat fix_ctl_win -f

@sinsharat
Copy link
Contributor Author

@xiang90 thank you so much for helping me out on this. Really grateful for the help and patience you showed.
@heyitsanthony Thanks for your help and support.

@sinsharat sinsharat closed this Sep 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants