-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add support to run pre/post scripts #1673
Conversation
Pull Request Test Coverage Report for Build 11368553812Details
💛 - Coveralls |
0de4ded
to
ae7b205
Compare
91ba861
to
b7c35c1
Compare
e4feeaf
to
52b2fd8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor comments, looks good to me.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could move them to a different place (in AutoYaST they are copied to /var/adm/autoinstall
.
"properties": { | ||
"pre": { | ||
"title": "Pre-installation scripts", | ||
"description": "User-defined scripts to run before the installation starts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before the post-partitioning. I can improve the description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They were expected to be used to modify the profile, but I am not sure whether we should implement that behavior. Or perhaps to activate some hardware.
.await?; | ||
} | ||
|
||
self.client.run_scripts(ScriptsGroup::Pre).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that's a good question. When would you run the pre-script? In the future we should be able to run the script when loading the profile (to modify it in-place). But we can postpone it until you click the "install" button. I guess we will need to do more adjustments in this area.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would do it one level up, when you load profile, then run any pre script it will find....but e.g. not before you validate that profile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, no, question was not where in the code, but when. Given that the pre scripts does not support changing the profile yet, I am not sure there is a use case for them right now.
Perhaps you want to enable something, but then pre scripts might need to run the probing, but probing happens only once in Agama by now (and for single product scenarios it happens too early).
This reverts commit 73f830f.
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 edit
command.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