Fixes #21383: Add logic to handle campaign within Rudder #4366

VinceMacBuche · 2022-07-07T14:56:50Z

https://issues.rudder.io/issues/21383

VinceMacBuche · 2022-07-07T15:12:24Z

Commit modified

VinceMacBuche · 2022-07-07T15:28:29Z

Commit modified

VinceMacBuche · 2022-07-07T15:31:20Z

Commit modified

VinceMacBuche · 2022-07-08T10:31:01Z

Commit modified

fanf · 2022-07-08T15:55:39Z

webapp/sources/rudder/rudder-core/src/main/resources/reportsSchema.sql

+, eventid     text
+, state       text
+, startDate   timestamp with time zone NOT NULL
+, endDate   timestamp with time zone NOT NULL


I think we will want a type (or kind) colum to be able to do select * from campaign where kind=dist-upgrade once we will have several compaign type (and i will avoid a migration script later on).
We also discussed a compliance (result?) field which would a summary of the campaign results and avoid join request and computation with the campaign-specific table. Likely json with a number of success/error.

I do think result should be apart from events

fanf · 2022-07-08T16:02:06Z

...es/rudder/rudder-core/src/main/scala/com/normation/rudder/campaigns/CampaignRepository.scala

+  }
+  def get(id : CampaignId) : IOResult[Campaign] = {
+    for {
+      content <- IOResult.effect ("error when getting "){


missing end of message ?

fanf · 2022-07-08T16:02:49Z

...es/rudder/rudder-core/src/main/scala/com/normation/rudder/campaigns/CampaignRepository.scala

+  def save(c : Campaign): IOResult[Campaign] = {
+    for {
+      file <- IOResult.effect {
+        val file = path / (c.info.id.value ++".json")


one too many +

fanf · 2022-07-08T16:03:56Z

...es/rudder/rudder-core/src/main/scala/com/normation/rudder/campaigns/CampaignRepository.scala

+
+class CampaignRepositoryImpl(campaignSerializer: CampaignSerializer) extends CampaignRepository {
+
+  val path =  root / "var" / "rudder" /" configuration-repository" / "campaigns"


We want that to be a parameter to be able to change it for tests

fanf · 2022-07-08T16:04:45Z

...es/rudder/rudder-core/src/main/scala/com/normation/rudder/campaigns/CampaignRepository.scala

+    for {
+      file <- IOResult.effect {
+        val file = path / (c.info.id.value ++".json")
+        file.createFileIfNotExists(true)


can you add createParents =, it helps understanding what the true is

fanf · 2022-07-08T16:07:41Z

webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/campaigns/DataTypes.scala

+  def info : CampaignInfo
+  def details : CampaignDetails
+}
+


We will need a big warning message here: "All the data type define here are part of the json API. If you change them, change them in a forward compatible way, or seal revision in dedicated case class + skim"

fanf · 2022-07-08T16:09:51Z

...es/rudder/rudder-core/src/main/scala/com/normation/rudder/campaigns/JSONReportsHandler.scala

+  }
+
+  def start() = {
+    loop.delay(5.seconds).forever


doesn't it need a .forkDaemon ?

fanf · 2022-07-08T16:18:15Z

webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/campaigns/DataTypes.scala

+case class CampaignEvent(id : CampaignEventId, campaignId : CampaignId, state : CampaignEventState, start : DateTime, end : DateTime )
+case class CampaignEventId(value : String)
+
+sealed trait CampaignEventState


please use the conventional way:

add a def value: String in trait

add an object CampaignEventState to encapsulate case objects

add a def values = sealarate.IforgotTheName[CampaignEventState]

create a def parse(s: String) = values.find(_ == s)

If it breaks your json serialisation, you can have an implicit class ToValue(c: CampaignEventState) { def value = c match { case Scheduled => "scheduled" ... }

But it's important that the string representation of the values and their parsing is done here, and nowhere else.
Same: if you need to use string representation in db queries or whatever, use Scheduled.value in place.
It's more searchable, and it's easier to see what needs to be updated if one day we need to change a value (and we prefer to not do it, but futur is complicated)

fanf · 2022-07-08T16:19:56Z

...udder/rudder-core/src/main/scala/com/normation/rudder/campaigns/JsonCampaignSerializer.scala

+
+  implicit val idDecoder : JsonDecoder[CampaignId] = JsonDecoder[String].map(s => CampaignId(s))
+
+  val iso8601 = DateTimeFormat.forPattern("yyyy-MM-dd'T'HH:mm:ssZZ")


I think we have it in DateFormaterService, else we need to add it;

fanf · 2022-07-08T16:21:06Z

...udder/rudder-core/src/main/scala/com/normation/rudder/campaigns/JsonCampaignSerializer.scala

+    } catch {
+      case e: Exception => Left(e.getMessage)
+    }
+  )


actually, that devoder should be in dateformatter service (but that's for an other day, because zio json is not at that level of pom.xml IIRC)

fanf · 2022-07-08T16:21:55Z

...udder/rudder-core/src/main/scala/com/normation/rudder/campaigns/JsonCampaignSerializer.scala

+      case 3 => Right(Third)
+      case -1 => Right(Last)
+      case -2 => Right(SecondLast)
+      case _ => Left(s"value should be between [-2,3], received ${v}")


0 is not a valid case, can you adapt message?

fanf · 2022-07-08T16:22:26Z

...udder/rudder-core/src/main/scala/com/normation/rudder/campaigns/JsonCampaignSerializer.scala

+      case Skipped => "skipped"
+      case Running =>  "running"
+      case Finished => "finished"
+      case Scheduled => "scheduled"


it needs to be Running.value as explained above

VinceMacBuche · 2022-07-08T16:23:13Z

Commit modified

fanf

Appart the little remarks, LGTM, you can merge it when you feel ready

VinceMacBuche · 2022-07-11T22:39:43Z

Commit modified

VinceMacBuche · 2022-07-11T23:17:27Z

Commit modified

Normation-Quality-Assistant · 2022-07-12T09:49:36Z

OK, merging this PR

VinceMacBuche requested a review from fanf July 7, 2022 14:56

VinceMacBuche force-pushed the bug_21383/add_logic_to_handle_campaign_within_rudder branch 2 times, most recently from bc4ee62 to eaff027 Compare July 7, 2022 15:00

VinceMacBuche mentioned this pull request Jul 7, 2022

Campaign - WIP #4319

Closed

VinceMacBuche force-pushed the bug_21383/add_logic_to_handle_campaign_within_rudder branch from eaff027 to 5364c47 Compare July 7, 2022 15:12

VinceMacBuche force-pushed the bug_21383/add_logic_to_handle_campaign_within_rudder branch from 5364c47 to b522b8c Compare July 7, 2022 15:28

VinceMacBuche force-pushed the bug_21383/add_logic_to_handle_campaign_within_rudder branch 2 times, most recently from 1e5b13b to e0e78b4 Compare July 8, 2022 10:31

fanf reviewed Jul 8, 2022

View reviewed changes

VinceMacBuche force-pushed the bug_21383/add_logic_to_handle_campaign_within_rudder branch from e0e78b4 to 541669b Compare July 8, 2022 16:23

fanf reviewed Jul 8, 2022

View reviewed changes

VinceMacBuche force-pushed the bug_21383/add_logic_to_handle_campaign_within_rudder branch 2 times, most recently from d99b297 to 1ca8770 Compare July 11, 2022 22:55

Fixes #21383: Add logic to handle campaign within Rudder

d855f09

VinceMacBuche force-pushed the bug_21383/add_logic_to_handle_campaign_within_rudder branch from 1ca8770 to d855f09 Compare July 11, 2022 23:17

VinceMacBuche added the Ready for merge label Jul 12, 2022

Normation-Quality-Assistant merged commit d855f09 into Normation:master Jul 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #21383: Add logic to handle campaign within Rudder #4366

Fixes #21383: Add logic to handle campaign within Rudder #4366

VinceMacBuche commented Jul 7, 2022

VinceMacBuche commented Jul 7, 2022

VinceMacBuche commented Jul 7, 2022

VinceMacBuche commented Jul 7, 2022

VinceMacBuche commented Jul 8, 2022

fanf Jul 8, 2022

VinceMacBuche Jul 11, 2022

fanf Jul 8, 2022

fanf Jul 8, 2022

fanf Jul 8, 2022

fanf Jul 8, 2022

fanf Jul 8, 2022

fanf Jul 8, 2022

fanf Jul 8, 2022

fanf Jul 8, 2022

fanf Jul 8, 2022

fanf Jul 8, 2022

fanf Jul 8, 2022

VinceMacBuche commented Jul 8, 2022

fanf left a comment

VinceMacBuche commented Jul 11, 2022

VinceMacBuche commented Jul 11, 2022

Normation-Quality-Assistant commented Jul 12, 2022


		class CampaignRepositoryImpl(campaignSerializer: CampaignSerializer) extends CampaignRepository {

		val path = root / "var" / "rudder" /" configuration-repository" / "campaigns"


		implicit val idDecoder : JsonDecoder[CampaignId] = JsonDecoder[String].map(s => CampaignId(s))

		val iso8601 = DateTimeFormat.forPattern("yyyy-MM-dd'T'HH:mm:ssZZ")

Fixes #21383: Add logic to handle campaign within Rudder #4366

Fixes #21383: Add logic to handle campaign within Rudder #4366

Conversation

VinceMacBuche commented Jul 7, 2022

VinceMacBuche commented Jul 7, 2022

VinceMacBuche commented Jul 7, 2022

VinceMacBuche commented Jul 7, 2022

VinceMacBuche commented Jul 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VinceMacBuche commented Jul 8, 2022

fanf left a comment

Choose a reason for hiding this comment

VinceMacBuche commented Jul 11, 2022

VinceMacBuche commented Jul 11, 2022

Normation-Quality-Assistant commented Jul 12, 2022