Avoid build on linera net up --kubernetes #2986
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
ndr-ds
force-pushed
the
11-28-avoid_build_on_linera_net_up_--kubernetes
branch
from
b6242ce to
a872d17
Compare
ndr-ds
force-pushed
the
11-28-avoid_build_on_linera_net_up_--kubernetes
branch
from
a872d17 to
37758e3
Compare
| /// Don't build docker image. This assumes that the image is already built. | ||
| #[cfg(feature = "kubernetes")] | ||
| #[arg(long, default_value = "false")] | ||
| no_build: bool, | ||
|
|
||
| /// The name of the docker image to use. | ||
| #[cfg(feature = "kubernetes")] | ||
| #[arg(long, default_value = "linera:latest")] | ||
| docker_image_name: String, | ||
|
|
There was a problem hiding this comment.
What's the link with the kubernetes feature and why --kubernetes in the title of this PR?
There was a problem hiding this comment.
--kubernetes calls the kubernetes version of linera net up, which needs to be compiled with the kubernetes feature. But if you don't compile with the kubernetes feature, the --kubernetes option is not even available.
You can still compile with the kubernetes feature and run just linera net up without --kubernetes, and it won't run the kubernetes version
There was a problem hiding this comment.
I see. I misread the test plan (which could have mentioned these extra arguments)
There was a problem hiding this comment.
nit: It's hard to understand the context because the motivation section doesn't mention kubernetes (and locally superficially seems contradictory although now I remember we have kind).
There was a problem hiding this comment.
Good point! I edited the motivation, hopefully it's clearer now
|
Rather than adding a single parameter, I think it would be more Kubernetes-idiomatic to provide some arbitrary YAML configuration file that gets patched onto the relevant Kubernetes configuration. |
@Twey I'm not sure I get it 🤔 Are you talking about the |
| @@ -17,7 +17,7 @@ impl DockerImage { | |||
| &self.name | |||
| } | |||
|
|
|||
| pub async fn build(name: String, binaries: &BuildArg, github_root: &PathBuf) -> Result<Self> { | |||
| pub async fn build(name: &String, binaries: &BuildArg, github_root: &PathBuf) -> Result<Self> { | |||
| fn new( | ||
| network: Network, | ||
| testing_prng_seed: Option<u64>, | ||
| binaries: BuildArg, | ||
| no_build: bool, | ||
| docker_image_name: String, |
There was a problem hiding this comment.
If this is &str (I don't think you need ownership) wouldn't it make life easier (no cloning everywhere)?
All the futures are joined at the end of the function so there are no borrows which will outlive the reference (I think).
ndr-ds
force-pushed
the
11-28-avoid_build_on_linera_net_up_--kubernetes
branch
from
37758e3 to
1b117c3
Compare
ndr-ds
changed the base branch from
12-03-remotenet_-_existingnet
to
graphite-base/2986
ndr-ds
force-pushed
the
11-28-avoid_build_on_linera_net_up_--kubernetes
branch
from
1b117c3 to
6266ccd
Compare
ndr-ds
force-pushed
the
graphite-base/2986
branch
from
3a4b51d to
909b3e7
Compare
ndr-ds
force-pushed
the
11-28-avoid_build_on_linera_net_up_--kubernetes
branch
from
6266ccd to
3650595
Compare
|
Will merge this for now, but @Twey lmk if you have any reservations with this approach, and I can refine it in a follow up PR |
Motivation
When testing kubernetes related config changes locally with
linera net up --kubernetes, there's no way to just use an existing docker image (if you're not testing something on the linera binaries, there's no need to rebuild the docker image every time)Proposal
Add options to skip building the image and just use an existing one. The name of the image to use also became a configurable option
Test Plan
Ran
linera net up --kuberneteslocally with these new argumentsRelease Plan