-
Notifications
You must be signed in to change notification settings - Fork 96
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
Implement YAML support for imposters #72
Implement YAML support for imposters #72
Conversation
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.
It looks quite good, great job 👍
I left few comments. Ping me once you're done.
Thanks!!!
internal/server/http/imposter.go
Outdated
|
||
const ( | ||
jsonImposterExtension = ".imp.json" | ||
yamlImposterExtension = ".imp.yml" |
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.
According to YAML's official FAQs, the official extension is .yaml
.
However, I know .yml
is also widely used.
So, I'd suggest you to add support for both file extensions.
internal/server/http/server.go
Outdated
if err != nil { | ||
log.Printf("error trying to load %s imposter: %v", f, err) | ||
log.Printf("error trying to load %v imposter: %v", config, err) |
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'd keep the file path here, as it's easier to see which imposter is failing.
internal/server/http/server.go
Outdated
s.addImposterHandler(imposters, f) | ||
log.Printf("imposter %s loaded\n", f) | ||
s.addImposterHandler(imposters, config) | ||
log.Printf("imposter %v loaded\n", 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.
Same for the file path.
internal/server/http/server.go
Outdated
@@ -134,9 +135,9 @@ func (s *Server) Shutdown() error { | |||
return nil | |||
} | |||
|
|||
func (s *Server) addImposterHandler(imposters []Imposter, imposterFilePath string) { | |||
func (s Server) addImposterHandler(imposters []Imposter, config ImposterConfig) { |
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.
Nitpick: I'd use imposterConfig
as the variable name, instead. The same way you did for unmarshalImposters
method.
|
||
var parseError error | ||
|
||
switch imposterConfig.Type { |
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'd be defensive here and I'd add a default
clause here with a proper error. Otherwise, if something weird happens, we'll omit the error which is something not desired in terms of UX.
@@ -0,0 +1,7 @@ | |||
--- |
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.
Please, extend this example with all the valid attributes in order to ensure all of them works properly with YAML.
Co-authored-by: Joan López de la Franca Beltran <joanjan14@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #72 +/- ##
==========================================
+ Coverage 92.85% 93.11% +0.26%
==========================================
Files 8 8
Lines 238 247 +9
==========================================
+ Hits 221 230 +9
Misses 13 13
Partials 4 4
Continue to review full report at Codecov.
|
…xtended yaml examples
ping @joanlopez |
@joanlopez ping! :P |
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.
Looks nice 👌 Thanks!!
PS: @aperezg feel free to merge it!
Hello, I implemented YAML support for imposters based on #21.