Fixes #28040: Migrate CheckRudderGlobalParam to zio-json in rudder-webapp #6824
Conversation
P4uline
changed the title
|
PR updated with a new commit |
|
PR rebased |
P4uline
force-pushed
the
arch_28040/migrate_checkrudderglobalparam_to_zio_jon_in_rudder_webapp
branch
from
257ee9c to
10dd842
Compare
|
PR updated with a new commit |
|
PR rebased |
P4uline
force-pushed
the
arch_28040/migrate_checkrudderglobalparam_to_zio_jon_in_rudder_webapp
branch
from
2660d2c to
cb5cc0f
Compare
| /** | ||
| * Parse a JSON JValue to ConfigValue. It always succeeds. | ||
| */ | ||
| @Deprecated(since = "8.0", forRemoval = true) |
There was a problem hiding this comment.
Not sure actually if the migration to zio-json started on version 8.0...
There was a problem hiding this comment.
The migration started a long time ago (7.0.0 IIRC)
Why wouldn't it be 9.1 ?
This would be removed when we migrate the whole Properties.scala, that I hope is before the 9.1 release...
|
|
||
| // lift json need that to be topevel | ||
| // TODO: handle the following TODO | ||
| // TODO: add Option[String] revision in API |
There was a problem hiding this comment.
@fanf I would like to create a redmine ticket to handle to delete the TODO
// TODO: add Option[String] revision in API
Just need a bit of context
clarktsiory
left a comment
clarktsiory
left a comment
There was a problem hiding this comment.
I only have a few suggestions, I'm open to discuss these few aspects !
| case Right(Json.Arr(list)) => | ||
| ZIO.foreach(list)(v => { | ||
| v.as[GlobalPropertiesJson] match { | ||
| case Left(error) => | ||
| Unexpected( | ||
| s"An unexpected error occurred while decoding json global properties $v as $GlobalPropertiesJson: $error" | ||
| ).fail | ||
| case Right(value) => IOResult.attempt(value.toGlobalParam) | ||
| } | ||
|
|
||
| }) | ||
| case _ => | ||
| Inconsistency( | ||
| s"Resources `$resource` must contain an array of json object with keys name, description, value." | ||
| ).fail |
There was a problem hiding this comment.
With our error-handling utils (in the ZioCommons file, e.g. EitherToIoResult#toIO), we have multiple ways to have a fluent syntax, instead of pattern matching (which at some point could hurt readability).
Here it could be something like :
value.map(_.asArray)
.toIO
.notOptional(s"Resources `$resource` must contain an array of json object with keys name, description, value.")
.flatMap(arr =>
ZIO.foreach(...)(/* can also be fluent */)
)| } | ||
| object GlobalPropertiesJson { | ||
| implicit val decoder: JsonDecoder[GlobalPropertiesJson] = DeriveJsonDecoder.gen[GlobalPropertiesJson] | ||
| } |
There was a problem hiding this comment.
now this is more verbose than derives JsonDecoder, I would happier with the derives syntax since the case class is already a JSON one 😄
| /** | ||
| * Parse a JSON JValue to ConfigValue. It always succeeds. | ||
| */ | ||
| @Deprecated(since = "8.0", forRemoval = true) |
There was a problem hiding this comment.
The migration started a long time ago (7.0.0 IIRC)
Why wouldn't it be 9.1 ?
This would be removed when we migrate the whole Properties.scala, that I hope is before the 9.1 release...
2 participants
https://issues.rudder.io/issues/28040
fromJsonValuewith a nowarn the remaining calls