Skip to content
This repository was archived by the owner on Nov 22, 2024. It is now read-only.

Runner config from the CLI#1014

Merged
andreaTP merged 17 commits into
masterfrom
runnerconfig-to-cli
Apr 9, 2021
Merged

Runner config from the CLI#1014
andreaTP merged 17 commits into
masterfrom
runnerconfig-to-cli

Conversation

@andreaTP

@andreaTP andreaTP commented Apr 5, 2021

Copy link
Copy Markdown
Contributor

This is the second library shared across tools and core if we start to have more we should merge back the two sbt projects.

TODO:

  • test it once more (it and runLocal)
  • go through the open TODO with @RayRoestenburg
  • regenerate the GraalVM configuration as soon as names are settled
  • fix the release process to release the correspondent artifact

@andreaTP andreaTP requested a review from RayRoestenburg April 5, 2021 15:56
@andreaTP andreaTP marked this pull request as ready for review April 6, 2021 17:17
Comment thread core/build.sbt
libraryDependencies ++= Vector(JacksonScalaModule)
)
.settings(
Compile / unmanagedSourceDirectories += (ThisProject / baseDirectory).value / ".." / ".." / "tools" / "cloudflow-runner-config" / "src" / "main" / "scala",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's chat about tools / core again ;-)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

back to the core !

import cloudflow.blueprint.deployment.ApplicationDescriptorJsonFormat._
import cloudflow.blueprint.RunnerConfigUtils._
import cloudflow.streamlets.{ BooleanValidationType, DoubleValidationType, IntegerValidationType, StreamletExecution, StreamletLoader }
import cloudflow.runner

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strange import?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I see now, rather just use cloudflow.runner... instead of runner... in code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed


def makeConfig: Try[Config] = Try {
val configFilePathString = Option(System.getProperty("config.file")).getOrElse(s"$ConfigMountPath/$ConfigFile")
val configFilePathString = Option(System.getProperty("config.file")).getOrElse(s"$ConfigSecretMountPath/$ConfigFile")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is slightly confusing, maybe we can chat.

case (name, topic) =>
name -> runner.config.Topic(id = topic.id,
// TODO: check with Ray the default
cluster = topic.cluster.getOrElse(""),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default is None

val configPath = Paths.get(configFilePathString)
val secretPath = Paths.get(s"$ConfigSecretMountPath/$SecretConfigFile")

val applicationConfig = if (Files.exists(configPath)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment here that this is for backwards compat or extract a function called backwardsCompatConfig

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

},
portMappings = Option(deployment.portMappings).getOrElse(Map.empty).map {
case (name, pm) =>
// TODO: check with Ray: cluster should be "default"?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same, default is None

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@JsonProperty("id")
id: String,
@JsonProperty("cluster")
cluster: String,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@RayRoestenburg RayRoestenburg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some changes suggested.

case (name, pm) =>
// TODO: check with Ray: cluster should be "default"?
name -> runner.config.Topic(id = pm.id, cluster = pm.cluster.getOrElse(""), config = pm.config)
name -> cloudflow.runner.config.Topic(id = pm.id, cluster = pm.cluster.getOrElse(""), config = pm.config)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why still getOrElse?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is outdated

@andreaTP andreaTP force-pushed the runnerconfig-to-cli branch from 9ee6edb to 296f5a9 Compare April 8, 2021 18:13
@andreaTP

andreaTP commented Apr 9, 2021

Copy link
Copy Markdown
Contributor Author

should be ready to go now @RayRoestenburg

@andreaTP andreaTP merged commit 8b736a4 into master Apr 9, 2021
@andreaTP andreaTP deleted the runnerconfig-to-cli branch April 9, 2021 09:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants