MF-1265 - Make Transformer type configurable #1331
Conversation
dborovcanin
force-pushed
the
configurable-transformer
branch
from
aa02b77
to
ce81b53
Compare
Codecov Report
@@ Coverage Diff @@
## master #1331 +/- ##
==========================================
- Coverage 59.89% 59.87% -0.03%
==========================================
Files 113 113
Lines 8835 8835
==========================================
- Hits 5292 5290 -2
- Misses 3081 3083 +2
Partials 462 462
Continue to review full report at Codecov.
|
| @@ -156,6 +162,21 @@ func newService(session *gocql.Session, logger logger.Logger) consumers.Consumer | |||
| return repo | |||
| } | |||
|
|
|||
| func makeTransformer(cfg config, logger logger.Logger) transformers.Transformer { | |||
There was a problem hiding this comment.
whould it make sense that this function is actually in Transformer interface?
There was a problem hiding this comment.
This can't be extracted to the Transformer interface because it would cause circular dependency (implementation imports Transformer to implement it and Transformer imports implementations so that it can instantiate them).
| case "SENML": | ||
| logger.Info("Using SenML transformer") | ||
| return senml.New(cfg.contentType) | ||
| case "JSON": |
There was a problem hiding this comment.
should this be a constant?
There was a problem hiding this comment.
It's used only here, so I'd say it's not necessary (it only adds a couple of extra lines of code).
There was a problem hiding this comment.
I agree with @mteodor
Maybe we can export it from /pkg/messaging? Currently is used only here yes, but also in all writers and probably will be used in other places too. In general, it would be cool to call it messaging.format.senml or similar.
There was a problem hiding this comment.
That makes sense, but it looks like it's related to this comment. I'm not sure if we should put these constants in pkg/messaging/transformer and have implementation info on the same location where the interface is, or in a separate configuration package. We have a lot of repetitive code in cmd and we should probably fix this in a separate PR attached to issue #1286.
There was a problem hiding this comment.
@dusanb94 Yea I agree about transformers, but I don't think it's useful only for configuration, either related to transformers.
IMHO it's a simple string, a format type definition that we support (in your PR used for config and related to transformers yes, but not neccearly), It actually belongs to messaging in general (it defines message format type). That's why I see it in /pkg/messaging as exported available format, currently a type string, easily changeable to any other format.
| return json.New() | ||
| default: | ||
| logger.Error(fmt.Sprintf("Can't create transformer: unknown transformer type %s", cfg.transformer)) | ||
| os.Exit(1) |
There was a problem hiding this comment.
I agree, SenML should be also default
There was a problem hiding this comment.
Default is probably defined by ENV constant, maybe it is not bad to report error here and give user / deployment engineer info that ENV var has not be set.
There was a problem hiding this comment.
Yes, SenML is default and it makes sense to put it here, but the problem is that SenML requires format when instantiation, so defaulting (no env set) would also imply that we set a default format. I'm afraid it's too many implicit default values.
There was a problem hiding this comment.
Agree, exit is ok here. Better to exit then trick me to use a format that I didn't want it. It's too critical IMHO
| return json.New() | ||
| default: | ||
| logger.Error(fmt.Sprintf("Can't create transformer: unknown transformer type %s", cfg.transformer)) | ||
| os.Exit(1) |
There was a problem hiding this comment.
Default is probably defined by ENV constant, maybe it is not bad to report error here and give user / deployment engineer info that ENV var has not be set.
dborovcanin
dismissed stale reviews from manuio, drasko, and nmarcetic
via
4a3253b
dborovcanin
force-pushed
the
configurable-transformer
branch
from
ce81b53
to
4a3253b
Compare
Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com>
dborovcanin
changed the title
Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com>
Signed-off-by: dusanb94 dusan.borovcanin@mainflux.com
What does this do?
This pull request enables configuring the Transformer type for the Mainflux writer through environment variables.
Which issue(s) does this PR fix/relate to?
Resolves #1265.
List any changes that modify/break current functionality
There are no such breaks.
Have you included tests for your changes?
No.
Did you document any new/modified functionality?
Yes.