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

Switch from go/flags to spf13/cobra #27

Merged
merged 2 commits into from Mar 12, 2019

Conversation

Projects
None yet
2 participants
@RaviTezu
Copy link
Contributor

RaviTezu commented Feb 24, 2019

Signed-off-by: RaviTezu ravi-teja@live.com

Description/Changes

This PR replaces the go/flags package with spf13/cobra for parsing the command line arguments:

  • Moved parse_upstream.go code into the cmd/client.go as we are do the parsing only in the client mode. Thoughts?
  • Introduced pkg/errors to wrap the context around the errors which are retuned. Thoughts? We can just log/Print them as we are currently doing, no strong feelings here. But I think, wrapping errors using pkg/errors will help in future as the codebase bigger and bigger.
  • Updated the README.md file with the changes introduced to the commands.

How Has This Been Tested?

Looks like we are yet to add the unit tests, so I have tested the changes by manually running the commands and see how the arguments are being parsed in client and server modes.

How are existing users impacted? What migration steps/scripts do we need?

Yes, there's a change in the way the existing users run the commands. For example, -server=true/false has been split into two subcommands like inlets server and inlets client. I have updated the README.md with the changes. I think, we will need to bump up the version? Feel free to let me know I will update this PR with bumping up the version.

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • read the CONTRIBUTION guide
  • signed-off my commits with git commit -s

Adding [WIP] to the PR title as there are some open questions and will remove it once we have the answers.
Closes #7

@derek derek bot added the new-contributor label Feb 24, 2019

@RaviTezu

This comment has been minimized.

Copy link
Contributor Author

RaviTezu commented Feb 24, 2019

@alexellis I see the build is failing as I have moved the code in parse_upstream.go to cmd/client.go. I will wait for the your reply before making any changes to the travis config file.

@alexellis

This comment has been minimized.

Copy link
Owner

alexellis commented Feb 24, 2019

Why does the build fail? Can you fix it by editing Dockerfile?

@derek

This comment has been minimized.

Copy link

derek bot commented Feb 24, 2019

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --sign-off and then git push --force.

@derek derek bot added the no-dco label Feb 24, 2019

@RaviTezu RaviTezu force-pushed the RaviTezu:replace_flags_with_cobra branch from a9fea35 to 66f1502 Feb 24, 2019

@derek derek bot removed the no-dco label Feb 24, 2019

@RaviTezu RaviTezu force-pushed the RaviTezu:replace_flags_with_cobra branch from 66f1502 to 7cfc5e9 Feb 24, 2019

@RaviTezu

This comment has been minimized.

Copy link
Contributor Author

RaviTezu commented Feb 24, 2019

@alexellis Yes, fixed it by editing the Docker file.

Short: "Start the tunnel client.",
Long: `Start the tunnel client.
Example: inlets client -remote=192.168.0.101:80 -upstream=http://127.0.0.1:3000

This comment has been minimized.

@alexellis

alexellis Feb 26, 2019

Owner

This has the old syntax, should be --, not -

return errors.Wrap(err, "failed to get 'print-token' value.")
}

if len(token) >0 && printToken{

This comment has been minimized.

@alexellis

alexellis Feb 26, 2019

Owner

You should run gofmt -w -s on these files and rebase the commit.

return nil
}

func init() {

This comment has been minimized.

@alexellis

alexellis Feb 26, 2019

Owner

Move to top of file?

README.md Outdated
- "-upstream=http://gateway.openfaas:8080"
- "-remote=your-public-ip"
- "client"
- "--upstream=http://gateway.openfaas:8080"

This comment has been minimized.

@alexellis

alexellis Feb 26, 2019

Owner

How about giving an example with multiple --upstream?

@alexellis

This comment has been minimized.

Copy link
Owner

alexellis commented Feb 26, 2019

FYI we now have a -version command to add as inlets version after merging #17

Alex

@RaviTezu RaviTezu force-pushed the RaviTezu:replace_flags_with_cobra branch from 7cfc5e9 to e4cd6b1 Mar 1, 2019

@RaviTezu

This comment has been minimized.

Copy link
Contributor Author

RaviTezu commented Mar 1, 2019

@alexellis Included the version changes as well.

Show resolved Hide resolved README.md Outdated
Show resolved Hide resolved README.md
Show resolved Hide resolved cmd/client.go Outdated
Show resolved Hide resolved cmd/client.go
@alexellis
Copy link
Owner

alexellis left a comment

I can see you've accidentally merged a merge conflict <<< HEAD or similar. I've also added some notes. Please could you revise and ping me? This is very close to getting the merge now and is a big improvement.

@RaviTezu RaviTezu force-pushed the RaviTezu:replace_flags_with_cobra branch from e4cd6b1 to 42fb3c6 Mar 3, 2019

@RaviTezu RaviTezu changed the title [WIP] Switch from go/flags to spf13/cobra Switch from go/flags to spf13/cobra Mar 3, 2019

@RaviTezu

This comment has been minimized.

Copy link
Contributor Author

RaviTezu commented Mar 3, 2019

@alexellis Thanks for the review. Pushed the changes.

@alexellis

This comment has been minimized.

Copy link
Owner

alexellis commented Mar 6, 2019

Awesome, I'll give it another pass soon and hopefully get this merged/released.

@alexellis

This comment has been minimized.

Copy link
Owner

alexellis commented Mar 6, 2019

Have we also updated the service entries for systemd?

@derek

This comment has been minimized.

Copy link

derek bot commented Mar 7, 2019

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --sign-off and then git push --force.

@derek derek bot added the no-dco label Mar 7, 2019

@RaviTezu RaviTezu force-pushed the RaviTezu:replace_flags_with_cobra branch from 0c2723a to a374fb7 Mar 7, 2019

@derek derek bot removed the no-dco label Mar 7, 2019

@derek

This comment has been minimized.

Copy link

derek bot commented Mar 7, 2019

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --sign-off and then git push --force.

@derek derek bot added the no-dco label Mar 7, 2019

@RaviTezu RaviTezu force-pushed the RaviTezu:replace_flags_with_cobra branch from 6e41ba9 to 839668e Mar 7, 2019

@derek derek bot removed the no-dco label Mar 7, 2019

@RaviTezu

This comment has been minimized.

Copy link
Contributor Author

RaviTezu commented Mar 7, 2019

@alexellis Updated the systemd file.

@alexellis
Copy link
Owner

alexellis left a comment

LGTM

@alexellis

This comment has been minimized.

Copy link
Owner

alexellis commented Mar 11, 2019

We have some conflicts with the base branch which means I cannot rebase your PR into master.

Please do the following to sync up with master and also to collapse all of your commits into a single commit, removing the accidental merge at the same time:

git checkout replace_flags_with_cobra
git checkout -b replace_flags_with_cobra_backup # make a backup first

git remote add upstream https://github.com/alexellis/inlets
git fetch upstream

git checkout replace_flags_with_cobra
git reset HEAD~4 # where 4 is your number of changes / commits
git add .
git stash # temporarily stash your changes

git rebase upstream/master # important to use rebase here
git stash pop # get your changes back

# Review all files for merge conflicts and fix them if needed
# When I did this, I had to fix a conflict in hack/inlets.service
# Fix it and anything else needed.

git add .
git commit -s # now add your message for the whole set of changes
git push origin replace_flags_with_cobra --force # sync the PR

Thank you, looking forward to merging this.

@alexellis

This comment has been minimized.

Copy link
Owner

alexellis commented Mar 11, 2019

@RaviTezu FYI

switch from go/flags to spf13/cobra
Signed-off-by: RaviTezu <ravi-teja@live.com>

@RaviTezu RaviTezu force-pushed the RaviTezu:replace_flags_with_cobra branch from 839668e to e66d73f Mar 11, 2019

@RaviTezu

This comment has been minimized.

Copy link
Contributor Author

RaviTezu commented Mar 11, 2019

@alexellis Done

@RaviTezu

This comment has been minimized.

Copy link
Contributor Author

RaviTezu commented Mar 11, 2019

@alexellis I just did a docker build . in ~/go/src/github.com/alexellis/inlets and it worked. Can we trigger the check again?

@alexellis

This comment has been minimized.

Copy link
Owner

alexellis commented Mar 11, 2019

This is the error


12.49s$ make dist
CGO_ENABLED=0 GOOS=linux go build -ldflags "-s -w -X main.Version=0.5.7-7-g43d7c62 -X main.GitCommit=43d7c62b767746a0de5bb3e250582ac21fde78f3" -a -installsuffix cgo -o bin/inlets
# github.com/alexellis/inlets/cmd
cmd/client.go:13:2: undefined: inletsCmd
cmd/server.go:13:2: undefined: inletsCmd
make: *** [dist] Error 2
The command "make dist" exited with 2.

You can view the logs on Travis following the status link.

@RaviTezu

This comment has been minimized.

Copy link
Contributor Author

RaviTezu commented Mar 12, 2019

@alexellis Both the commands are working fine on my local machine. Something wrong with travis config?

╰─ make docker                                                                                                                                                                                                                                                                                                 
docker build --build-arg Version=0.5.7-6-ge66d73f --build-arg GIT_COMMIT=e66d73f50d1e41af0d966d0409ef5906bf507959 -t alexellis2/inlets:0.5.7-6-ge66d73f .
Sending build context to Docker daemon   30.7MB
Step 1/19 : FROM golang:1.10 as build
 ---> 6fd1f7edb6ab
Step 2/19 : WORKDIR /go/src/github.com/alexellis/inlets
 ---> Using cache
 ---> ec434f8df965
Step 3/19 : COPY .git               .git
 ---> Using cache
 ---> 0d88ba4f481e
Step 4/19 : COPY vendor             vendor
 ---> Using cache
 ---> 81355bbfc2d0
Step 5/19 : COPY pkg                pkg
 ---> Using cache
 ---> 16a07b7d4639
Step 6/19 : COPY cmd                cmd
 ---> Using cache
 ---> e478f0cacb2a
Step 7/19 : COPY main.go            .
 ---> Using cache
 ---> d1876458f660
Step 8/19 : ARG GIT_COMMIT
 ---> Using cache
 ---> 9a0d71a54a99
Step 9/19 : ARG VERSION
 ---> Using cache
 ---> ca89ee0635de
Step 10/19 : RUN CGO_ENABLED=0 go build -ldflags "-s -w -X main.GitCommit=${GIT_COMMIT} -X main.Version=${VERSION}" -a -installsuffix cgo -o /usr/bin/inlets
 ---> Running in 0ab8a7da5006
Removing intermediate container 0ab8a7da5006
 ---> 7ed8b1a7ddea
Step 11/19 : FROM alpine:3.9
 ---> 5cb3aa00f899
Step 12/19 : RUN apk add --force-refresh ca-certificates
 ---> Using cache
 ---> 295697f25b93
Step 13/19 : RUN addgroup -S app && adduser -S -g app app   && mkdir -p /home/app || :   && chown -R app /home/app
 ---> Using cache
 ---> bd4ca52535d9
Step 14/19 : COPY --from=build /usr/bin/inlets /usr/bin/
 ---> 94fa1e44772f
Step 15/19 : WORKDIR /home/app
 ---> Running in 38062e00d2ac
Removing intermediate container 38062e00d2ac
 ---> a5cf465fd776
Step 16/19 : USER app
 ---> Running in 6fe99b17ddde
Removing intermediate container 6fe99b17ddde
 ---> 1aa4ba3442d0
Step 17/19 : EXPOSE 80
 ---> Running in e1d7d8ad61c9
Removing intermediate container e1d7d8ad61c9
 ---> 2981f83062e9
Step 18/19 : ENTRYPOINT ["inlets"]
 ---> Running in 769e00bf0f9a
Removing intermediate container 769e00bf0f9a
 ---> 1473b1e04aa3
Step 19/19 : CMD ["-help"]
 ---> Running in a00d5ee27bda
Removing intermediate container a00d5ee27bda
 ---> 49fa109be872
[Warning] One or more build-args [Version] were not consumed
Successfully built 49fa109be872
Successfully tagged alexellis2/inlets:0.5.7-6-ge66d73f

╰─ make dist                                                                                                                                                                                                                                                                                                  
CGO_ENABLED=0 GOOS=linux go build -ldflags "-s -w -X main.Version=0.5.7-6-ge66d73f -X main.GitCommit=e66d73f50d1e41af0d966d0409ef5906bf507959" -a -installsuffix cgo -o bin/inlets
CGO_ENABLED=0 GOOS=darwin go build -ldflags "-s -w -X main.Version=0.5.7-6-ge66d73f -X main.GitCommit=e66d73f50d1e41af0d966d0409ef5906bf507959" -a -installsuffix cgo -o bin/inlets-darwin
CGO_ENABLED=0 GOOS=linux GOARCH=arm GOARM=6 go build -ldflags "-s -w -X main.Version=0.5.7-6-ge66d73f -X main.GitCommit=e66d73f50d1e41af0d966d0409ef5906bf507959" -a -installsuffix cgo -o bin/inlets-armhf
CGO_ENABLED=0 GOOS=linux GOARCH=arm64 go build -ldflags "-s -w -X main.Version=0.5.7-6-ge66d73f -X main.GitCommit=e66d73f50d1e41af0d966d0409ef5906bf507959" -a -installsuffix cgo -o bin/inlets-arm64
@RaviTezu

This comment has been minimized.

Copy link
Contributor Author

RaviTezu commented Mar 12, 2019

@alexellis I see the issue. .gitignore is causing the issues. Added an extra check to allow inlets.go file. Build should work fine now.

Change .gitignore to take inlets.go file into consideration
Signed-off-by: RaviTezu <ravi-teja@live.com>
@alexellis
Copy link
Owner

alexellis left a comment

LGTM

@alexellis alexellis merged commit 9678d96 into alexellis:master Mar 12, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alexellis

This comment has been minimized.

Copy link
Owner

alexellis commented Mar 12, 2019

Thank you so much, this PR looks 💯

There's one more that I think is important for us to look at next: #42

Also remind me when we have a logo & a sticker to post you one 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.