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

multi: Cleanup recent alt DNS names additions. #1493

Merged
merged 1 commit into from
Oct 11, 2018

Conversation

davecgh
Copy link
Member

@davecgh davecgh commented Oct 11, 2018

This cleans up code recently added to support the --altdnsnames flag and DCRD_ALT_DNSNAMES environment variable to match the project standards, simplify it, and correct some issues with standalone test binaries.

  • Gets rid of the setup func which had several issues:
    • It was creating a file that was not needed (and without even checking the error)
    • Since it was parsing flags and missing with os.Args in a func, it was not possible to individual run the test funcs without failure due to the additional flags
  • Performs the flag parsing and removal in an init func that only runs once when the tests are run (and before TestMain)
  • Avoids creating empty objects when nil will suffice
  • Uses full sentences and periods in the comment and properly spaces them out so they are consistent with the rest of the code
  • Adds comments to all of the added test functions as required by the code contribution guidelines
  • Closes the create temp files before trying to write to them in the cert generation
  • Defers the removal of the temp files directly after they are created so they are removed properly if the next one fails to create
  • Uses consistent camel case in the variable names

@davecgh davecgh added this to the 1.4.0 milestone Oct 11, 2018
Copy link
Member

@dajohi dajohi left a comment

Choose a reason for hiding this comment

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

ok

config_test.go Outdated Show resolved Hide resolved
@@ -12,7 +12,7 @@ import (
"bytes"
"crypto/x509"
"encoding/pem"
"flag"

Copy link
Member

Choose a reason for hiding this comment

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

extra line slipped through.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Resolved.

@davecgh davecgh force-pushed the rpcserver_cleanup_altdns_additions branch from 080133d to ed2fea4 Compare October 11, 2018 16:35
This cleans up code recently added to support the --altdnsnames flag and
DCRD_ALT_DNSNAMES environment variable to match the project standards,
simplify it, and correct some issues with standalone test binaries.

- Gets rid of the setup func which had several issues:
  - It was creating a file that was not needed (and without even
    checking the error)
  - Since it was parsing flags and missing with os.Args in a func, it
    was not possible to individual run the test funcs without failure
    due to the additional flags
- Performs the flag parsing and removal in an init func that only runs
  once when the tests are run (and before TestMain)
- Avoids creating empty objects when nil will suffice
- Uses full sentences and periods in the comment and properly spaces
  them out so they are consistent with the rest of the code
- Adds comments to all of the added test functions as required by the
  code contribution guidelines
- Closes the create temp files before trying to write to them in the cert
  generation
- Defers the removal of the temp files directly after they are created so
  they are removed properly if the next one fails to create
- Uses consistent camel case in the variable names
@davecgh davecgh force-pushed the rpcserver_cleanup_altdns_additions branch from ed2fea4 to 5b8450a Compare October 11, 2018 19:17
@davecgh davecgh merged commit 5b8450a into decred:master Oct 11, 2018
@davecgh davecgh deleted the rpcserver_cleanup_altdns_additions branch October 11, 2018 19:40
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.

3 participants