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

Embed file using golang embed filesystem #30

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yudhaputrama
Copy link

Changes

  • Binary output can be run at anywhere because template and assets already embed inside binary
  • User can change server port using flag -port default still 6969
  • User can specify log file using flag -log

@yudhaputrama
Copy link
Author

I prefer binary can run at any folder so I don't need to copy all the assets to same folder
can you help review this @gusaul ?

func main() {
flag.StringVar(&logfile, "log", "", "Specify log file")
Copy link
Owner

Choose a reason for hiding this comment

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

this should have default value to log/grpcox.log

f, err := os.OpenFile("log/grpcox.log", os.O_RDWR|os.O_CREATE|os.O_APPEND, 0666)
if err != nil {
log.Fatalf("error opening file: %v", err)
if logfile != "" {
Copy link
Owner

Choose a reason for hiding this comment

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

dont need to check if have default value

if value, ok := os.LookupEnv("BIND_ADDR"); ok {
addr = value
}
addr := fmt.Sprintf(":%d", port)
Copy link
Owner

@gusaul gusaul May 19, 2021

Choose a reason for hiding this comment

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

this is breaking changes, we use env variable as config now, its should not completely replace, just make it replace if only being set by flag args

@gusaul
Copy link
Owner

gusaul commented May 19, 2021

@yudhaputrama please check the comment

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

Successfully merging this pull request may close these issues.

2 participants