feat: add support to run pre/post scripts #1673
Conversation
Pull Request Test Coverage Report for Build 11368553812Details
💛 - Coveralls |
imobachgs
force-pushed
the
auto-scripts
branch
from
0de4ded
to
ae7b205
Compare
imobachgs
force-pushed
the
auto-scripts
branch
from
91ba861
to
b7c35c1
Compare
imobachgs
force-pushed
the
auto-scripts
branch
from
e4feeaf
to
52b2fd8
Compare
| require "fileutils" | ||
| logs_dir = File.join(Yast::Installation.destdir, "var", "log", "agama") | ||
| FileUtils.mkdir_p(logs_dir) | ||
| FileUtils.cp_r(SCRIPTS_DIR, logs_dir) |
There was a problem hiding this comment.
We should document that the user scripts are copied to the logs in the installed system. The user scripts might contain sensitive data (passwords, tokens, keys, certificates,...) and the users should be aware of this when attaching the logs to bugzilla or sending via email.
| "properties": { | ||
| "pre": { | ||
| "title": "Pre-installation scripts", | ||
| "description": "User-defined scripts to run before the installation starts", |
There was a problem hiding this comment.
Um, when exactly? Nothing done yet?
I mean later we might add a "post-partitioning" script so it should be clean whether at this point something is already done by Agama or the system has not been touched yet.
|
|
||
| it "copies the artifacts to the installed system" do | ||
| storage.finish | ||
| expect(File).to exist(File.join(tmp_dir, "mnt", "var", "log", "agama", "scripts")) |
There was a problem hiding this comment.
I'm not sure if the test should really touch the testing system (create and copy files). I do not see much advantage over just to expect to call FileUtils.cp_r.
| self.client.post_void("/scripts/run", &group).await | ||
| } | ||
|
|
||
| pub async fn scripts(&self) -> Result<Vec<Script>, ServiceError> { |
|
|
||
| pub async fn scripts(&self) -> Result<Vec<Script>, ServiceError> { | ||
| let scripts = self.client.get("/scripts").await?; | ||
| Ok(scripts) |
There was a problem hiding this comment.
cannot it be simple oneliner self.client.get("/scripts").await ?
| use super::ScriptError; | ||
|
|
||
| #[derive(Debug, Clone, Copy, PartialEq, strum::Display, Serialize, Deserialize)] | ||
| #[strum(serialize_all = "camelCase")] |
There was a problem hiding this comment.
I will need to read a bit more about strum. First time I met it.
| /// Script's body. Either the body or the URL must be specified. | ||
| pub body: Option<String>, | ||
| /// URL to get the script from. Either the body or the URL must be specified. | ||
| pub url: Option<String>, |
There was a problem hiding this comment.
this looks like bad representation in rust. I kind of expect here enum with either body or url content (which are just marked string). I am just not sure how hard it will be to express it in serde.
| } else if let Some(body) = &self.body { | ||
| write!(file, "{}", &body)?; | ||
| } | ||
| // FIXME: else: invalid script definition |
There was a problem hiding this comment.
as said above, should be already catched by json serializer and not here in code where you can rely on valid code.
|
|
||
| /// Runs the scripts in the given group. | ||
| /// | ||
| /// They run in the order they were added to the repository. |
There was a problem hiding this comment.
I would document here that it does not return error if script failed. and in this case just log error.
| let body: Vec<u8> = std::fs::read(&path).unwrap(); | ||
| let body = String::from_utf8(body).unwrap(); | ||
| assert_eq!("error\n", body); | ||
| } |
| pub body: Option<String>, | ||
| /// URL to get the script from. Either the body or the URL must be specified. | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub url: Option<String>, |
There was a problem hiding this comment.
I still hope that serde can handle if we have it as enum...especially as it is really exclusive as having both body and url does not make sense.
| } | ||
|
|
||
| pub async fn load(&self) -> Result<ScriptsConfig, ServiceError> { | ||
| // TODO: Ok(self.client.scripts().await.unwrap()) |
There was a problem hiding this comment.
? I do not get this todo
| } | ||
|
|
||
| pub fn new_with_client(client: ScriptsClient) -> Result<Self, ServiceError> { | ||
| Ok(Self { client }) |
There was a problem hiding this comment.
how it can fail? this looks kind of strange.
| .await?; | ||
| } | ||
|
|
||
| self.client.run_scripts(ScriptsGroup::Pre).await?; |
There was a problem hiding this comment.
Hmm, I am not sure I like it. Separation of load and store is understandable, but why it runs pre script when store? This looks kind of unexpected.
| url: script.url.clone(), | ||
| body: script.body.clone(), | ||
| } | ||
| } |
There was a problem hiding this comment.
I have to say that this to_something looks kind of strange in rust. What is reason to not implement trait Into?
| ) -> Result<(), ScriptServiceError> { | ||
| let scripts = state.scripts.write().await; | ||
| if let Err(error) = scripts.run(group).await { | ||
| tracing::error!("Could not run user-defined scripts: {error}"); |
There was a problem hiding this comment.
so it means that it will return 200 when coulld not run scripts? I do not feel it is good behavior.
This is the first implementation of pre/post-installation scripts. They are not as capable as their AutoYaST counterparts, but the idea is to evolve them in the future.
Specifying a script
Warning
JSON does not support multiline values (a.k.a. blocks) but Jsonnet does it. Perhaps we should consider using Jsonnet in the
agama config editcommand.{ scripts: { pre: [ { name: "say-hi", body: ||| #!/bin/bash echo "before starting the installation" ||| } ] } }Note
What about having a plain list where each script has a
type(orgroup) likepre,post, etc.?When scripts get executed
Implementation details
The scripting support is written in Rust and does not use the YaST scripts code anymore. We will rewrite the Manager in Rust (at least the Agama-specific part) in the future, and the scripting support will be part of that.
Future
Agama scripts will gain additional features soon:
chroot.Pending tasks