Skip to content
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

Use YQ to process YAML image templates #2912

Merged
merged 2 commits into from
Nov 28, 2022

Conversation

mikem-zed
Copy link
Contributor

@mikem-zed mikem-zed commented Nov 8, 2022

it is a pain to manually update those patches. YQ can do the job, however I couldn't figure out how to handle one case:
in yaml files for Adam we insert a service in the middle of the array like following

      image: NEWLOGD_TAG
      cgroupsPath: /eve/services/newlogd
      oomScoreAdj: -999
+   - name: adam
+     image: lfedge/adam:latest
+     binds:
+        - /var/persist:/persist
+        - /var/config:/config
+     command: ["/bin/eve-embedded.sh"]
    - name: edgeview
      image: EDGEVIEW_TAG
      cgroupsPath: /eve/services/eve-edgeview

It seem YQ cannot do things like that, I can only add an element to the end or the beginning of the array. At least I couldn't figure out proper expression. Any Idea?

At least one commit form this PR can be taken as is: we can get rid of .sed files and ye YQ to patch the version

UPDATE:
I have switched to python version of YQ (https://kislyuk.github.io/yq/)
container for it is not available but can be build with following Dockerfile with following command

docker buildx build --platform linux/amd64,linux/arm64 .

FROM alpine:3.16 AS builder
RUN apk add --no-cache py-pip jq bash && pip install yq==3.1.0

RUN mkdir /workdir
WORKDIR /workdir

ENTRYPOINT ["/usr/bin/yq"]

I'm not sure what is the best way to manage it. Check in Dockerfile and build from Makefile? push it do docker repo? which one?

UPDATE 2: I have pushed yq to my public docker hub repo but idk what are our policies for that

@mikem-zed mikem-zed force-pushed the mikem/image-defs-yaml-yq branch from 325ea42 to 4429b40 Compare November 8, 2022 20:58
Copy link
Contributor

@giggsoff giggsoff left a comment

Choose a reason for hiding this comment

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

It seem YQ cannot do things like that, I can only add an element to the end or the beginning of the array. At least I couldn't figure out proper expression. Any Idea?

In our particular case it does not matter, services section processed without strict order, and all services start in parallel, so we can put adam in the end.

Maybe slicing around the index of interest, joining with new element and re-constructing of array will work, not sure.

It would be nice to have small document/comment somewhere what to do if we want to insert new element in rootfs.yml.in and avoid having it with mini HV (as a use-case we have in CI).

Also It seems reasonable to share prettified files as a commit of this PR.

#!/bin/bash

# reformat base template
yq -i '.' images/rootfs.yml.in
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we can wrap yq into docker run, as we did it in compose-image-yml.sh. For example adding

yq() {
  docker run --rm -i -v "${PWD}":/workdir mikefarah/yq "$@"
}

on top of the file works for me and do not require to have yq installed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this script will gone is final version becasue we will process rootfs.yml.in directly

@@ -3,7 +3,7 @@
set -e

yq() {
docker run --rm -i -v "${PWD}":/workdir mikefarah/yq "$@"
docker run --env-file ./env.list --rm -i -v "${PWD}":/workdir mikefarah/yq "$@"
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see that we call yq not only from patch_version but also from process-image-template should we fill ./env.list there as well?

@@ -0,0 +1 @@
.services += {"name": "kvm-tools","image": "KVMTOOLS_TAG", "cgroupsPath": "/eve/services/kvmtoo"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be kvmtool in the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure! thanks!

@mikem-zed
Copy link
Contributor Author

It seem YQ cannot do things like that, I can only add an element to the end or the beginning of the array. At least I couldn't figure out proper expression. Any Idea?

In our particular case it does not matter, services section processed without strict order, and all services start in parallel, so we can put adam in the end.

Maybe slicing around the index of interest, joining with new element and re-constructing of array will work, not sure.

I've got my question answered at https://stackoverflow.com/questions/74368503/is-it-possible-to-insert-an-element-into-a-middle-of-array-in-yaml-using-yq I still need to try this

It would be nice to have small document/comment somewhere what to do if we want to insert new element in rootfs.yml.in and avoid having it with mini HV (as a use-case we have in CI).

Also It seems reasonable to share prettified files as a commit of this PR.

There will be only one rootfs.yml.in file in final version and a few *.yq files. no need for other files

@mikem-zed mikem-zed force-pushed the mikem/image-defs-yaml-yq branch from 4429b40 to 4e167e1 Compare November 9, 2022 22:51
@mikem-zed mikem-zed requested a review from giggsoff November 9, 2022 23:00
@mikem-zed mikem-zed force-pushed the mikem/image-defs-yaml-yq branch 2 times, most recently from 57a3d70 to 7fdd3e9 Compare November 10, 2022 01:59
@giggsoff
Copy link
Contributor

I'm not sure what is the best way to manage it. Check in Dockerfile and build from Makefile? push it do docker repo? which one?

Can we try to put the binary into https://github.com/lf-edge/eve/blob/master/build-tools/src/scripts/Dockerfile and implement another target in Makefile using GOBUILDER/DOCKER_GO.

@mikem-zed
Copy link
Contributor Author

I'm not sure what is the best way to manage it. Check in Dockerfile and build from Makefile? push it do docker repo? which one?

Can we try to put the binary into https://github.com/lf-edge/eve/blob/master/build-tools/src/scripts/Dockerfile and implement another target in Makefile using GOBUILDER/DOCKER_GO.

Potentially I can add a target to build-tools/src/scripts/Dockerfile to build this container, but why we cannot just pull it from my docker repo? we did it for another version of yq. It is much easier than mess with our Makefile, But may be I just do not understand your suggestion. BTW, I had to manually generate patches again for another PR :(. We really need this automated way to edit image configs

@mikem-zed
Copy link
Contributor Author

pushed a new version of container for both arm64 and amd64

@mikem-zed mikem-zed changed the title [WIP] Use YQ to process YAML image templates Use YQ to process YAML image templates Nov 16, 2022
@mikem-zed mikem-zed force-pushed the mikem/image-defs-yaml-yq branch from 7fdd3e9 to 832d478 Compare November 16, 2022 18:35
@eriknordmark eriknordmark requested a review from deitch November 18, 2022 19:43
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

Looks like there is a conflict now.
I can't tell from the discussion whether this is otherwise ready to merge as a first phase.

@deitch
Copy link
Contributor

deitch commented Nov 20, 2022

So the purpose here is to make it easier to update all of the patch files? Whereas the patch files are sensitive to precise line numbers, and thus need to be updated with each change in the file-to-be-patched (rootfs.yml.in), yq will just work on the logical structure, and avoid the need to be updated unless that particular .yq file needs to be update?

Makes sense.

Copy link
Contributor

@deitch deitch left a comment

Choose a reason for hiding this comment

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

This might be a lot easier if linuxkit supported multiple files, like:

lkt build rootfs.yml override1.yml override2.yml

The problem is that the override.yml files would need to have clear syntax: when am I overriding a section vs appending to it, etc.?

So we probably should go for this for now, but @mikem-zed if you feel like taking a crack at getting this into linuxkit, that would be great.

YQ is very good at processing YAML and formatting patches manually is a
pain.

Signed-off-by: Mikhail Malyshev <mikem@zededa.com>
The container can be rebuilt if necessary using the following
command and Dockerfile

docker buildx build --platform linux/amd64,linux/arm64 .

FROM alpine:3.16 AS builder
RUN apk add --no-cache py-pip jq bash && pip install yq==3.1.0

RUN mkdir /workdir
WORKDIR /workdir

ENTRYPOINT ["/usr/bin/yq"]

Signed-off-by: Mikhail Malyshev <mikem@zededa.com>
@mikem-zed mikem-zed force-pushed the mikem/image-defs-yaml-yq branch from 832d478 to e91a11f Compare November 21, 2022 11:46
@mikem-zed
Copy link
Contributor Author

rebased to resolve conflicts

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

LGTM

@deitch
Copy link
Contributor

deitch commented Nov 25, 2022

As pointed out in comments on #2626, this needs an update of the documentation of how to use it? E.g. docs should describe something like:

To build eve, run `make eve`. To build eve with option A and option B and option C, run `make OPT=A OPT=B eve` (or whatever is correct).

and how that works, so someone does not have to reverse engineer it in the future.

@mikem-zed
Copy link
Contributor Author

As pointed out in comments on #2626, this needs an update of the documentation of how to use it? E.g. docs should describe something like:

To build eve, run `make eve`. To build eve with option A and option B and option C, run `make OPT=A OPT=B eve` (or whatever is correct).

and how that works, so someone does not have to reverse engineer it in the future.

this PR doesn't introduce ANY changes from the user perspective. We just get rid of patch files. The list of build targets is still fixed and remains the same. However #2626 can be updated after we merge this one to leverage YQ. it will solve a problem with exponential explosion of number of patch files

@eriknordmark eriknordmark merged commit 61bf4f6 into lf-edge:master Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants