[WIP] Read YAML configuration settings #29
Conversation
Codecov Report
@@ Coverage Diff @@
## master #29 +/- ##
==========================================
- Coverage 70.04% 68.97% -1.08%
==========================================
Files 6 6
Lines 434 448 +14
==========================================
+ Hits 304 309 +5
- Misses 102 110 +8
- Partials 28 29 +1
Continue to review full report at Codecov.
|
lib/config.go
Outdated
| @@ -47,6 +50,15 @@ type Config struct { | |||
| baseStore remoteStore | |||
| } | |||
|
|
|||
| // yamlConfig specifies the optional settings taken from `.s3deploy.yml` | |||
| type yamlConfig struct { | |||
There was a problem hiding this comment.
The code in the PR looks correct, but since we already have a fileConfig struct and code that reads that from a YAML file, we should try to reuse that code. So I suggest that you just add these new fields to the same struct. And I suspect there should be some related tests you can expand on.
There was a problem hiding this comment.
Thanks for the review, I'm working on implementing your suggestions. :)
YAML file is now loaded into Config struct, unnecessary struct is removed, code for readSettings() simplified.
lib/config_test.go
Outdated
|
|
||
| cfg, err := flagsToConfig(flags) | ||
| assert.NoError(err) | ||
| assert.NoError(flags.Parse([]string{"-source=testdata/.s3deploy.yml"})) |
There was a problem hiding this comment.
This is not a good location because 'testdata' contains the data used by deployer_test.go. Is there a location suited for the test file?
lib/config.go
Outdated
| } | ||
|
|
||
| // Load in settings, but only when the accompanying command flag hasn't been set |
There was a problem hiding this comment.
The above sentence should be respected.
There was a problem hiding this comment.
So if the user choose to add his/her AWS key and secret to the config file, the general AWS credentials still take precedence?
I tried several times to program that situation this week, but it's unfortunately outside my current experience to implement. So just checking here if I actually understood correctly what you're saying.
There was a problem hiding this comment.
If I say
s3deploy -source=dist/ -region=eu-west-1 -bucket=bepsays.comI want those values to be used, watever values are stored in the config file for those settings.
There was a problem hiding this comment.
Ah I see, thanks for clarifying. I was looking in the wrong direction.
For what you ask, the code works like that for me. This is my test .s3deploy.yml file:
bucket: example.com
key: xcsds
secret: xdsfdsf
region: us-west
source: public
But I can override these settings with the command flags. When I use the latest commit above (18ac3a0) it goes like so.
Without command flags, it uses us-west as the region and example.com for the bucket name:
C:\Users\Jos\go\src\github.com\bep\s3deploy>go run main.go -try
s3deploy v2, commit none, built at unknown
This is a trial run, with no remote updates.
Total in 0.35 seconds
error: RequestError: send request failed
caused by: Get https://s3.us-west.amazonaws.com/example.com: dial tcp: lookup s3.us-west.amazonaws.com: no such host
But with command flags those settings are overruled:
C:\Users\Jos\go\src\github.com\bep\s3deploy>go run main.go -try -bucket=myexamplewebsite.com -region=europe-512
s3deploy v2, commit none, built at unknown
This is a trial run, with no remote updates.
Total in 0.37 seconds
error: RequestError: send request failed
caused by: Get https://s3.europe-512.amazonaws.com/myexamplewebsite.com: dial tcp: lookup s3.europe-512.amazonaws.com: no such host
Now it looked for the myexamplewebsite.com in the region europe-512, thereby overriding the settings specified in the s3deploy.yml file.
Edit
This behaviour seems to happen because of the unmarshal() function that yaml.Unmarshal() uses.
This reverts commit bcc4bcd.
Moved test of settings file in separate /lib/ subdirectory, fixed assert.Equal() function call
|
With the latest commit I've moved the |
This is the first step for #23. It's my first Go pull request, so I'm also marking it as 'work in progress' to get feedback for what you'd like to see differently. I look forward to anything that makes it better. 🙂
Things that remain on my task list after this commit:
.s3deploy.yml. (I thought about including such a warning in the code, but printing such a message each time feels inappropriate to users.)I tested the code on
go version go1.9 windows/amd64.