-
Notifications
You must be signed in to change notification settings - Fork 696
feature: "yq" provision mode #3892
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
base: master
Are you sure you want to change the base?
Conversation
Why not just apt-get yq in a shell script? |
I think this is too specialized to be built into Lima given that you can already do this with a regular provisioning script: provision:
- mode: user
script: |
if ! command -v yq >/dev/null 2>&1; then
apt-get update
apt install -y yq
fi
mkdir -p ...
touch ...
yq ... |
Because |
Ok, that is unfortunate. I was wondering why it was dragging in Python. But |
If the guestagent was already using And we would rather want to bring down the size of the guestagents, so I don't think we should be doing that either. |
Dragging in the Python implementation makes a sense, as it seems more "authentic" version |
Fair enough. Unfortunately on homebrew |
cc10a18
to
6cb8a7e
Compare
If we want to support adding large local files to the cidata volume, then I would suggest the way to do this would be to extend the base: template://docker
provision:
- mode: data
path: /usr/local/bin/yq
file: ~/Downloads/yq_linux_arm
permissions: 755 It didn't work because of the tilde path (I just created a PR to fix that), but also because template embedding would attempt to include the file as a base64 encoded string, which exceeds the (somewhat arbitrary) max size of 4MB we have for I still think that once created a Lima instance should not reference other local files outside the instance directory. We could get rid of the file size limit (not a good idea, imo), or we could have some mechanism that files would be copied into a cache directory inside the instance directory, and the file reference would be rewritten to point to it. I don't think the use case is particularly strong though when you can also just fetch the file from GitHub in a provisioning script. We do this with all manner of other prerequisites as well. But I can see the utility when you want to bundle something that is not available for download. |
This PR adds |
6cb8a7e
to
06b81fd
Compare
provisionTool:
yq:
- location: "~/Downloads/yq_linux_amd64"
arch: "x86_64"
digest: "sha256:..." The example above is from |
@norio-nomura : I think you should add an issue for what you are trying to accomplish here, before diving straight into the implementation and the PR? My gut feeling is that this will meet the same fate as the "ansible" provisioning. |
I know this is just an example, but I just realized that you are editing a JSON file, so you can just use |
base: template://docker
provision:
- mode: user
script: |
set -eux -o pipefail
DAEMON="{{.Home}}/.config/docker/daemon.json"
mkdir -p "$(dirname "$DAEMON")"
[[ ! -e $DAEMON ]] && echo "{}" > "$DAEMON"
chmod 644 "$DAEMON"
EXPR='.features["containerd-snapshotter"] = {{.Param.containerdSnapshotter}}'
jq "$EXPR" "$DAEMON" >"$DAEMON.new"
mv "$DAEMON.new" "$DAEMON"
param:
containerdSnapshotter: true Seems to work fine: ❯ limactl start --yes --name snap ./snap.tmpl
❯ limactl shell snap sh -c "cat ~/.config/docker/daemon.json"
{
"features": {
"containerd-snapshotter": true
}
} |
Unrelated, but I also think we should just always use the Even Docker Desktop has been using it by default for over a year now for all new installations. It is not considered experimental anymore. |
Yup, I also have some I want to make those tasks much simpler like this PR does. |
If I make an issue before I write a PR, I expect to receive an answer like #3892 (comment) It came. Being able to make |
06b81fd
to
bb0337b
Compare
Added - mode: yq
path: /etc/systemd/system/docker.service.d/override.conf
format: ini
expression: .Socket.SocketUser="{{.User}}" |
9a9c7c4
to
420f6ee
Compare
That is OK, but it also runs into the risk of 1) not realizing there is a simpler way to accomplish the same thing and 2) risking that the PR is never merged - or maybe merged and then deprecated and removed again, like Ansible |
I like the I think a better approach would be to bundle I've created #3908 to show how simple this is to implement. What do you think about using that approach @norio-nomura? |
420f6ee
to
03e90a4
Compare
03e90a4
to
84e10a3
Compare
The |
I will separate the |
84e10a3
to
08c6556
Compare
updated:
|
a71bd73
to
8f59a28
Compare
```yaml # `yq` creates or edits a file in the guest filesystem by using `${LIMA_CIDATA_GUEST_INSTALL_PREFIX}/bin/lima-guestagent yq`. # `${LIMA_CIDATA_GUEST_INSTALL_PREFIX}/bin/lima-guestagent yq` has the same functionality as https://github.com/mikefarah/yq. # The file specified by `path` will be updated with the specified `expression` using `yq --inplace`. # If the file does not exist, it will be created using `yq --null-input`. # `format` is optional and defaults to "auto", which is passed to `yq` by `--input-format <format>`, `--output-format <format>`. # One of the following is expected to work: # "auto", "a", "ini", "i", "json", "j", "props", "p", "xml", "x", "yaml", "y" # `yq` v4.47.1 can write .ini, .json, .properties, .xml, .yaml, and .yml files. # See https://github.com/mikefarah/yq for more info. # Any missing directories will be created as needed using `mkdir -p`. # The file permissions will be set to the specified value using `chmod`. # The file and directory creation will be performed with the specified user privileges. # If the existing file is not writable by the specified user, the operation will fail. # `path` and `expression` are required. # `user` and `permissions` are optional. Defaults to "root" and 644. - mode: yq path: "{{.Home}}/.config/docker/daemon.json" expression: ".features.containerd-snapshotter = {{.Param.containerdSnapshotter}}" format: auto user: "{{.User}}" permissions: 644 Signed-off-by: Norio Nomura <norio.nomura@gmail.com>
8f59a28
to
71563bd
Compare
@@ -57,6 +57,7 @@ declare -A CHECKS=( | |||
["mount-path-with-spaces"]="" | |||
["provision-data"]="" | |||
["param-env-variables"]="" | |||
["provision-yq"]="" |
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.
This is a tiny nitpick, so please ignore unless there are other changes needed:
I would keep the provision-data
and provision-yq
tests close together in the source because they are related (both provisioning tests).
@@ -77,6 +77,64 @@ if [ -d "${LIMA_CIDATA_MNT}"/provision.data ]; then | |||
done | |||
fi | |||
|
|||
if [ -d "${LIMA_CIDATA_MNT}"/provision.yq ]; then | |||
yq="${LIMA_CIDATA_GUEST_INSTALL_PREFIX}/bin/lima-guestagent yq" | |||
if ${yq} --version >/dev/null; then |
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 don't think this test is needed since yq
is built into the guest agent.
if [ -d "${LIMA_CIDATA_MNT}"/provision.yq ]; then | ||
yq="${LIMA_CIDATA_GUEST_INSTALL_PREFIX}/bin/lima-guestagent yq" | ||
if ${yq} --version >/dev/null; then | ||
for expression in "${LIMA_CIDATA_MNT}"/provision.yq/*; do |
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 find expression
misleading, as it really is a filename. All the other provisioning loops use f
, so I think this should use the same:
for expression in "${LIMA_CIDATA_MNT}"/provision.yq/*; do | |
for f in "${LIMA_CIDATA_MNT}"/provision.yq/*; do |
format=$(deref "LIMA_CIDATA_YQ_PROVISION_${filename}_FORMAT") | ||
path=$(deref "LIMA_CIDATA_YQ_PROVISION_${filename}_PATH") | ||
permissions=$(deref "LIMA_CIDATA_YQ_PROVISION_${filename}_PERMISSIONS") | ||
user=$(deref "LIMA_CIDATA_YQ_PROVISION_${filename}_USER") |
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 think the yq
provision mode should use owner
instead of user
to match the data
provision mode. The path
and permissions
properties use the same name, but user
is different from owner
, which is confusing.
user=$(deref "LIMA_CIDATA_YQ_PROVISION_${filename}_USER") | |
owner=$(deref "LIMA_CIDATA_YQ_PROVISION_${filename}_OWNER") |
Of course this applies to all other places in the PR too.
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.
user
of mode: yq
and owner
of mode: data
behave a little differently.
For the directory created by mkdir -p
in mode: data
, root will be the owner regardless of the owner
.
If you create {{.Home}}/.config/docker/daemon.json
with mode: data
, sudo chown -R {{. User}} ~/.config
is required for mode: user
.
If user
is aligned with owner
, I want to align the behavior of mkdir -p
of mode: data
so that it is executed by the user of owner
. 🤔
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.
Maybe mode: data
should be changed to do the same thing mode: yq
does now for mkdir -p
? It makes no sense to me that these 2 modes would behave differently.
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'll change to remove user
and use owner
instead.
I'll align the behavior of mode: data
.
owner
also includes group, but it is difficult to apply group to the directory to be created with mkdir -p
alone.
I will probably leave a comment as TODO and use mkdir -p
as it is. 🤔
@@ -77,6 +77,64 @@ if [ -d "${LIMA_CIDATA_MNT}"/provision.data ]; then | |||
done | |||
fi | |||
|
|||
if [ -d "${LIMA_CIDATA_MNT}"/provision.yq ]; then | |||
yq="${LIMA_CIDATA_GUEST_INSTALL_PREFIX}/bin/lima-guestagent yq" |
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.
Guestagent is only installed when LIMA_CIDATA_PLAIN
is not set. I assume the provisioning mode should work for plain instance too, so need to execute it from the cidata mount point.
Also need to make sure the guestagent is included in the CIDATA iso for plain instances if there is at least one yq
provisioning entry.
yq="${LIMA_CIDATA_GUEST_INSTALL_PREFIX}/bin/lima-guestagent yq" | |
yq="${LIMA_CIDATA_MNT}/lima-guestagent yq" |
for _, ptr := range []*string{ | ||
// mode: data | ||
p.Content, p.Owner, p.Path, |
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.
Good catch of the additional templates. Maybe add &p.Script
as well, then you can eliminate the check above as well.
# # `yq` creates or edits a file in the guest filesystem by using `${LIMA_CIDATA_GUEST_INSTALL_PREFIX}/bin/lima-guestagent yq`. | ||
# # `${LIMA_CIDATA_GUEST_INSTALL_PREFIX}/bin/lima-guestagent yq` has the same functionality as https://github.com/mikefarah/yq. |
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 think that is too much internals knowledge. Just say it uses yq
. We already use yq
in other places, like the --set
CLI options.
# # `yq` creates or edits a file in the guest filesystem by using `${LIMA_CIDATA_GUEST_INSTALL_PREFIX}/bin/lima-guestagent yq`. | |
# # `${LIMA_CIDATA_GUEST_INSTALL_PREFIX}/bin/lima-guestagent yq` has the same functionality as https://github.com/mikefarah/yq. | |
# # Create or edit a file in the guest filesystem by using `yq`. |
# # `${LIMA_CIDATA_GUEST_INSTALL_PREFIX}/bin/lima-guestagent yq` has the same functionality as https://github.com/mikefarah/yq. | ||
# # The file specified by `path` will be updated with the specified `expression` using `yq --inplace`. | ||
# # If the file does not exist, it will be created using `yq --null-input`. | ||
# # `format` is optional and defaults to "auto", which is passed to `yq` by `--input-format <format>`, `--output-format <format>`. |
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 explain what it does, not how it is implemented.
# # `format` is optional and defaults to "auto", which is passed to `yq` by `--input-format <format>`, `--output-format <format>`. | |
# # `format` is optional and defaults to "auto". |
# # Any missing directories will be created as needed using `mkdir -p`. | ||
# # The file permissions will be set to the specified value using `chmod`. | ||
# # The file and directory creation will be performed with the specified user privileges. |
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.
# # Any missing directories will be created as needed using `mkdir -p`. | |
# # The file permissions will be set to the specified value using `chmod`. | |
# # The file and directory creation will be performed with the specified user privileges. | |
# # Any missing directories will be created as needed. | |
# # The file permissions will be set to the specified value. | |
# # The file and directory creation will be performed as the specified owner. |
@@ -68,3 +67,5 @@ message: | | |||
docker context use lima-{{.Name}} | |||
docker run hello-world | |||
------ | |||
param: | |||
containerdSnapshotter: false |
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 understand you probably don't want to change the default behaviour, but I would be supportive of changing the default to true
.
Are there any scenarios where this would break something?
based on #3908
updated:
yq
subcommands tolimactl
andlima-guestagent
#3908provisionTool