-
Notifications
You must be signed in to change notification settings - Fork 9
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
Remove env configuration #338
Conversation
@@ -0,0 +1,3 @@ | |||
source { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love having separate configs in the repo if it can be avoided - you'll see in my most recent PR I actually started removing some of them & using the docs examples instead.
The ones for testing the config logic itself might be necessary - or at least not easy to remove. But it feels like for these ones we might be able to find a different approach. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @colmsnowplow ! Removed most of the test configs that were added. A couple that remain could be also removed with some refactor of Init
, but sounded like out of scope for this PR.
t.Setenv("SOURCE_KINESIS_APP_NAME", appName) | ||
t.Setenv("TEST_KINESIS_STREAM_NAME", streamName) | ||
t.Setenv("TEST_KINESIS_REGION", testutil.AWSLocalstackRegion) | ||
t.Setenv("TEST_KINESIS_APP_NAME", appName) | ||
|
||
c, err := config.NewConfig() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for example here - perhaps we can make a new config object, then overwrite the source with our test's config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff!
I think it's ready to merge but I have a small ask for a QOL improvement to try to remove test configs if possible.
If that's not as easy as I assume it is let me know - it's mergeable now for sure :)
Already some pieces of the annoying parsing logic coming out - great to see!
config/decode.go
Outdated
@@ -34,33 +33,12 @@ type Decoder interface { | |||
// Decoders. The zero value of DecoderOptions means no-prefix/nil-input, | |||
// which should be usable by the Decoders. | |||
type DecoderOptions struct { | |||
Prefix string | |||
Input hcl.Body | |||
Input hcl.Body | |||
} | |||
|
|||
// envDecoder implements Decoder. | |||
type envDecoder struct{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this one can be removed too?
8e27d31
to
3572799
Compare
@colmsnowplow , @pondzix reverted back to running with defaults when no config file is provided, as we discussed. |
31a9b2f
to
df3f4e2
Compare
Jira ref: PDP-1247