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 dynamic port conf for restapiserver #802

Merged
merged 3 commits into from
Sep 28, 2021

Conversation

abdrd
Copy link
Contributor

@abdrd abdrd commented Sep 27, 2021

we can now pass port number as a command-line argument when running the application.

--port 5000, -port 5000, --port=5000, -port=5000, etc. are all valid arguments.

if a given port is not present, the server will start listening on 1323 by default.

Copy link
Member

@seokho-son seokho-son left a comment

Choose a reason for hiding this comment

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

Thank you @betelgeuse-7 👍

This PR works nicely!

I have a minor comment for validation.
Please check the review comment and update PR if necessary.

README.md Show resolved Hide resolved
Comment on lines +45 to +48
// giving a default value of "1323"
port := flag.String("port", "1323", "port number for the restapiserver to listen to")
flag.Parse()

Copy link
Member

Choose a reason for hiding this comment

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

How about adding validation for the given port number? [1-65535]

If you agree, you can refer to the code below.
I tested following code myself :)

Suggested change
// giving a default value of "1323"
port := flag.String("port", "1323", "port number for the restapiserver to listen to")
flag.Parse()
// giving a default value of "1323"
port := flag.String("port", "1323", "port number for the restapiserver to listen to")
flag.Parse()
// validate arguments from flag
validationFlag := true
// validation: port
// return TRUE if your number is in [1-65535] range
if portInt, err := strconv.Atoi(*port); err == nil {
if portInt < 1 || portInt > 65535 {
validationFlag = false
}
} else {
validationFlag = false
}
if !validationFlag {
fmt.Printf("%s is not a valid port number.\n", *port)
fmt.Printf("Please retry with a valid port number (ex: -port=[1-65535]).\n")
os.Exit(1)
}
$ ./cb-tumblebug -port=23242334

23242334 is not a valid port number.
Please retry with a valid port number (ex: -port=[1-65535]).
$ ./cb-tumblebug -port=123NiceContributor

123NiceContributor is not a valid port number.
Please retry with a valid port number (ex: -port=[1-65535]).

@seokho-son
Copy link
Member

@betelgeuse-7 I hope to know your opinion on the Makefile.

cb-tumblebug uses Makefile to build and run source code.

Do you think the Makefile needs to be enhanced
to allow the user to pass the -port argument when using make run?

@abdrd
Copy link
Contributor Author

abdrd commented Sep 27, 2021

@seokho-son
You are right. I should have added validation. Never trust user input :D
I will make new pr in 15 20 mins.

@abdrd
Copy link
Contributor Author

abdrd commented Sep 27, 2021

@betelgeuse-7 I hope to know your opinion on the Makefile.

cb-tumblebug uses Makefile to build and run source code.

Do you think the Makefile needs to be enhanced
to allow the user to pass the -port argument when using make run?

I haven't ever used Makefiles before. So unfortunately, I don't really have an opinion on that.

@seokho-son
Copy link
Member

/lgtm

@github-actions github-actions bot added the lgtm This PR is acceptable by at least one reviewer label Sep 27, 2021
@seokho-son
Copy link
Member

@jihoon-seo
Please check this PR and approve if LGTY ;)

(I hope to know your opinion regarding make run too)

@jihoon-seo
Copy link
Member

https://stackoverflow.com/questions/2214575/passing-arguments-to-make-run 을 참고하여

Makefile 을 다음과 같이 수정해 보았습니다.

default:
        go build -mod=mod -o cb-tumblebug
cc:
        GOOS=linux GOARCH=arm go build -mod=mod -o cb-tumblebug-arm
swag swagger:
        ~/go/bin/swag i -o ./api/rest/docs
proto protobuf pb:
        cd api/grpc/protobuf && $(MAKE) regenerate
cbadm:
        cd api/grpc/cbadm && $(MAKE)
run:
-        ./cb-tumblebug
+        ./cb-tumblebug $(ARGS)
clean:
        rm -v cb-tumblebug cb-tumblebug-arm
❯ make run --port 5432
make: unrecognized option '--port'

❯ make run --port=5432
make: unrecognized option '--port=5432'

make run 뒤에 적은 --port 5432 / --port=5432
./cb-tumblebug $(ARGS)$(ARGS) 로 넘어가는 것이 아니라
make 의 arg 로 인식되어 버립니다.

❯ make run PORT=5432  
./cb-tumblebug

...

⇨ http server started on [::]:1323
⇨ grpc server started on [::]:50252

이렇게 하면 ./cb-tumblebug PORT=5432 로 CB-TB가 실행되지만
이 PR에 의하면 --port 5000, -port 5000, --port=5000, -port=5000 등만이 유효한 입력이므로
입력한 5432가 아닌 기본값 1323 포트로 실행되었습니다.

일단 이 PR은 LGTMT 이므로 merge 하겠습니다. 😊

@jihoon-seo jihoon-seo merged commit 1ff5702 into cloud-barista:main Sep 28, 2021
@jihoon-seo
Copy link
Member

@all-contributors please add @betelgeuse-7 code

@allcontributors
Copy link
Contributor

@jihoon-seo

@betelgeuse-7 already contributed before to code

@abdrd
Copy link
Contributor Author

abdrd commented Sep 28, 2021

I just discovered that we can actually pass port parameter to make run easily.
if we change Makefile run command to this:
run: ./cb-tumblebug --port=$(PORT)

we can run the application as follows:
make run PORT=5000 (any port. not just 5000)

(
But this way we force the user to pass in a PORT. to prevent this we could add a secondary run method like: make runp with PORT parameter required.
ex:
runp: ./cb-tumblebug --port=$(PORT)
)

@jihoon-seo
Copy link
Member

@betelgeuse-7 Nice suggestion! 😊

[Suggestion]

default:
        go build -mod=mod -o cb-tumblebug
cc:
        GOOS=linux GOARCH=arm go build -mod=mod -o cb-tumblebug-arm
swag swagger:
        ~/go/bin/swag i -o ./api/rest/docs
proto protobuf pb:
        cd api/grpc/protobuf && $(MAKE) regenerate
cbadm:
        cd api/grpc/cbadm && $(MAKE)
run:
        ./cb-tumblebug
runwithport:
        ./cb-tumblebug --port=$(PORT)
clean:
        rm -v cb-tumblebug cb-tumblebug-arm

[Usage]

❯ make run
...
⇨ http server started on [::]:1323
⇨ grpc server started on [::]:50252

❯ make runwithport PORT=5432
...
⇨ http server started on [::]:5432
⇨ grpc server started on [::]:50252

@jihoon-seo jihoon-seo linked an issue Sep 28, 2021 that may be closed by this pull request
4 tasks
@jihoon-seo jihoon-seo mentioned this pull request Sep 28, 2021
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR is acceptable by at least one reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic config for server port
3 participants