search

aofei / air

An ideally refined web framework for Go.
https://pkg.go.dev/github.com/aofei/air
MIT License
441 stars 37 forks source link

debug flag #17

Closed SaulDoesCode closed 5 years ago

SaulDoesCode commented 5 years ago

Want to make running in debug_mode easier, adding a debug flag allows me to not have to edit the config file every time I want to debug.

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 364

Changes Missing Coverage Covered Lines Changed/Added Lines %
air.go 6 7 85.71%
<!-- Total: 6 7 85.71% -->
Totals Coverage Status
Change from base Build 363: 0.1%
Covered Lines: 732
Relevant Lines: 2444
💛 - Coveralls
coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 364

Changes Missing Coverage Covered Lines Changed/Added Lines %
air.go 6 7 85.71%
<!-- Total: 6 7 85.71% -->
Totals Coverage Status
Change from base Build 363: 0.1%
Covered Lines: 732
Relevant Lines: 2444
💛 - Coveralls
coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 364

Changes Missing Coverage Covered Lines Changed/Added Lines %
air.go 6 7 85.71%
<!-- Total: 6 7 85.71% -->
Totals Coverage Status
Change from base Build 363: 0.1%
Covered Lines: 732
Relevant Lines: 2444
💛 - Coveralls
SaulDoesCode commented 5 years ago

Also, consider using flaggy, since it allows you to set shorthand flags easilly.

var configfile string
flaggy.String(&configfile , "c", "config", "tell air where to look for the configuration file")

if configfile == "" {
   configfile = "config.toml"
}
myapp -c ./private/config.toml
SaulDoesCode commented 5 years ago

It would also be nice if you could set the config file's location programmatically.

air.Config("./private/config.toml") // ...reinterpret
SaulDoesCode commented 5 years ago

And there should be a warning/error printed out, when air can't find the config.toml, instead of exiting abruptly

SaulDoesCode commented 5 years ago

Out of frustration I've now made two config files, one for developing and another for production.

The Dev one has domain set to localhost, and, host_whitelist omitted so that there's no undesired redirects.

SaulDoesCode commented 5 years ago

Please make this autocert, ignore the tls_x_file values, just rely on autocert setting.

autocert = true # <- simple

tls_key_file="Let's Encrypt" # <- why both?
tls_cert_file="Let's Encrypt"

but it would be nice if autocert could be optionally disabled when in debug_mode, like my fork's DevAutoCert = false variable. Doing this would also require ignoring host_whitelist when in debug_mode with autocert disabled.

aofei commented 5 years ago

In this case, I think it would be more convenient to support a little more flags. Such as: --address, --tls-cert-file, --tls-key-file and other handy flags. What do you think?

aofei commented 5 years ago

It would also be nice if you could set the config file's location programmatically. ...

I don't think so. Dynamically reloading a configuration file is not an easy task. After all, we have too many configuration items to use only when starting the server. So, I think, the configurations should not support dynamic reloading. Or we add a func (such as Restart()) to restart the server. But that's too much.

SaulDoesCode commented 5 years ago

Inside air.go

var ConfigFile = "config.toml"

var called = false
func Init() {
 if called {
  return
}
called = true
 /* ... everything that was in init */ 
}

  if ConfigFile == "config.toml" {
   //   do everything the same
  } else {
  // ignore config related flags
  }
}

inside server.go

func Serve() {
  air.Init()
}

^_ It's a bit rough but what if we did something like this?

Only reason it might be nice to set these things programmatically is so that the main Application using air can take back control of the flags, or at least, offer sensible defaults without making the end user think about flags or even try to remember them. But it's your call...

SaulDoesCode commented 5 years ago

Yes, gimme all the flags you got, I love it!

aofei commented 5 years ago

Yeah, you are right, Air should not dominate the flags. I designed it in the first place to facilitate development and deployment, but now it looks... stupid. A framework should never control the flags.

aofei commented 5 years ago

Hi @SaulDoesCode,

I think adding a func Config(file string) error might be good, but should it be called automatically in the server#serve()?

The config.toml:

address = "localhost:6677"

The main.go:

package main

import "github.com/aofei/air"

func main() {
    air.Address = "localhost:7788"
    air.Serve()
}

The air.Address will be replaced with localhost:6677 if there is an implicit call in the air.Server().

SaulDoesCode commented 5 years ago

The user should not have to think about these things, just like the saying innocent until proven guilty, try to parse the config file, unless explicitly told not to.

I might go about it like this:

var HasConfig = true

// ... do everything the same, normal if HasConfig

// in user-land
air.HasConfig = false
air.Address = ":2443" 
air.AppName = "A Big Idea"
// ...more config options

// back in Air-land
if !HasConfig {
  // skip parsing
}

When HasConfig is false don't parse any files But if it's true, which is should be by default, then parse.

Though, perhaps, you should still retain a post-processing step for the config params, a Config Digest

// this is from another project
func digestConfig(config *Config) {
    if config.Private == "" {
        config.Private = "./private"
    }

    if config.Assets == "" {
        config.Assets = "./assets"
    }

    if config.Cache == "" {
        config.Cache = config.Private + "/cache"
    }

    if config.AutoCert && len(config.Whitelist) == 0 && config.Domain != "" {
        config.Whitelist = []string{config.Domain}
    }

       // [...more checks/defaults]
}

^- this project just maintains a global Config struct it can populate the values from a file or from a user's manual setting. But, it always run's the digest to check that values are valid

SaulDoesCode commented 5 years ago

Also, air.ConfigFile could be a useful option.

Then a user can set where to look before hand.

ConfigFile = flag.String("config", ConfigFile, "configuration file location")
aofei commented 5 years ago

I prefer air.Address more than air.Config.Address, so I gave up using a Config-like struct stuff.

See 026794c7d9949001ab85ea6bb6b17fc96ac0d907, I decided to use the ConfigFile. What do you think?

SaulDoesCode commented 5 years ago

For this project that also makes sense because air runs as a singleton, so there's no conflicting instances to worry about anyway. So yeah, that's fine.

SaulDoesCode commented 5 years ago

Awesome ConfigFile simplifies things a lot!