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.

Sign up for GitHub

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

Jump to bottom

Move config loading out of main and add tests for it #413

Merged
merged 1 commit into from
Feb 4, 2020
Merged

Move config loading out of main and add tests for it #413

merged 1 commit into from
Feb 4, 2020

Conversation

gabegorelick
Copy link
Contributor

The loading of config was previously untested.

Issue Type

  • Feature Pull Request

Summary

Move parseCommandLineFlags into config.go and add some tests for it.

Code contribution checklist

  1. I hereby allow the Copyright holder the rights to distribute this piece of
    code under any software license.
  2. The contribution fixes a single existing github issue, and it is linked
    to it.
  3. The code is as simple as possible, readable and follows the idiomatic Go
    guidelines.
  4. All new functionality is covered by automated test cases so the overall
    test coverage doesn't decrease.
  5. No issues are reported when running make full-test.
  6. Functionality not applicable to all users should be configurable.
  7. Configurations should be exposed through Lambda function environment
    variables which are also passed as parameters to the
    CloudFormation
    and
    Terraform
    stacks defined as infrastructure code.
  8. Global configurations set from the infrastructure stack level should also
    support per-group overrides using tags.
  9. Tags names and expected values should be similar to the other existing
    configurations.
  10. Both global and tag-based configuration mechanisms should be tested and
    proven to work using log output from various test runs.
  11. The logs should be kept as clean as possible (use log levels as
    appropriate) and formatted consistently to the existing log output.
  12. The documentation is updated to cover the new behavior, as well as the
    new configuration options for both stack parameters and tag overrides.
  13. A code reviewer reproduced the problem and can confirm the code
    contribution actually resolves it.

gabegorelick
gabegorelick commented Feb 2, 2020
View reviewed changes
autospotting.go
conf.LicenseType,
conf.PatchBeanstalkUserdata,
)
log.Printf("Configuration flags: %#v", conf)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pulled this change from #354.

codeclimate[bot]
codeclimate bot reviewed Feb 2, 2020
View reviewed changes
core/config.go
@@ -88,3 +93,103 @@ type Config struct {
// authentication method
PatchBeanstalkUserdata string
}

// ParseConfig loads configuration from command line flags, environments variables, and config files.
func ParseConfig(conf *Config) {
Copy link

Choose a reason for hiding this comment

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

Function ParseConfig has 85 lines of code (exceeds 50 allowed). Consider refactoring.

golangcibot
golangcibot reviewed Feb 2, 2020
View reviewed changes
core/config.go Outdated
"\tExample: ./AutoSpotting --patch_beanstalk_userdata true\n")

printVersion := flagSet.Bool("version", false, "Print version number and exit.\n")
flagSet.Parse(os.Args[1:])
Copy link

Choose a reason for hiding this comment

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

Error return value of flagSet.Parse is not checked (from errcheck)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

gabegorelick
gabegorelick commented Feb 2, 2020
View reviewed changes
core/config.go
@@ -88,3 +93,103 @@ type Config struct {
// authentication method
PatchBeanstalkUserdata string
}

// ParseConfig loads configuration from command line flags, environments variables, and config files.
func ParseConfig(conf *Config) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed from parseCommandLineFlags since it loads config from environment variables and config files too.

@gabegorelick gabegorelick force-pushed the config_test branch 2 times, most recently from 4129535 to acf5ea2 Compare February 2, 2020 23:55
codeclimate[bot]
codeclimate bot reviewed Feb 2, 2020
View reviewed changes
core/config.go
@@ -88,3 +93,106 @@ type Config struct {
// authentication method
PatchBeanstalkUserdata string
}

// ParseConfig loads configuration from command line flags, environments variables, and config files.
func ParseConfig(conf *Config) {
Copy link

Choose a reason for hiding this comment

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

Function ParseConfig has 87 lines of code (exceeds 50 allowed). Consider refactoring.

gabegorelick
gabegorelick commented Feb 3, 2020
View reviewed changes
core/config_test.go
"testing"
"time"

"gotest.tools/v3/assert"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can remove this, but it makes writing assertions a lot easier. It also has a package for testing binaries, which I think will be useful to get autospotting.go under test.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I'll check it out.

@gabegorelick gabegorelick marked this pull request as ready for review February 3, 2020 00:01
cristim
cristim reviewed Feb 3, 2020
View reviewed changes
Copy link
Member

@cristim cristim left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

core/config.go Outdated
conf.LogFlag = log.Ldate | log.Ltime | log.Lshortfile
conf.MainRegion = region
conf.SleepMultiplier = 1
conf.LicenseType = os.Getenv("LICENSE")
Copy link
Member

Choose a reason for hiding this comment

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

This looks unnecessary, it should be handled below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that was copied from the old parseCommandLineFlags. Updated.

@gabegorelick
Move config loading out of main and add tests for it
322c16a 
The loading of config was previously untested.
codeclimate[bot]
codeclimate bot reviewed Feb 4, 2020
View reviewed changes
core/config.go
@@ -88,3 +93,105 @@ type Config struct {
// authentication method
PatchBeanstalkUserdata string
}

// ParseConfig loads configuration from command line flags, environments variables, and config files.
func ParseConfig(conf *Config) {
Copy link

Choose a reason for hiding this comment

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

Function ParseConfig has 86 lines of code (exceeds 50 allowed). Consider refactoring.

@codeclimate
Copy link

codeclimate bot commented Feb 4, 2020

Code Climate has analyzed commit 322c16a and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

View more on Code Climate.

@cristim cristim merged commit 30a4392 into LeanerCloud:master Feb 4, 2020
@gabegorelick gabegorelick deleted the config_test branch February 4, 2020 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeclimate codeclimate[bot] codeclimate left review comments

@golangcibot golangcibot golangcibot left review comments

@cristim cristim cristim approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@gabegorelick @cristim @golangcibot