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.

Sign up for GitHub

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

Jump to bottom

Image: Update cXs images with missing python library #77

Closed
wants to merge 1 commit into from
Closed

Image: Update cXs images with missing python library #77

wants to merge 1 commit into from

Conversation

elkoniu
Copy link
Contributor

@elkoniu elkoniu commented Aug 22, 2024

No description provided.

@elkoniu elkoniu requested review from thozza and achilleas-k August 22, 2024 09:59
achilleas-k
achilleas-k approved these changes Aug 22, 2024
View reviewed changes
Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

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

❤️

achilleas-k
achilleas-k requested changes Aug 22, 2024
View reviewed changes
docker-bake.hcl Outdated Show resolved Hide resolved
@elkoniu elkoniu force-pushed the add-misisng-python-lib branch from 516a506 to 35a4508 Compare August 22, 2024 13:31
@elkoniu elkoniu requested a review from achilleas-k August 22, 2024 13:33
thozza
thozza reviewed Aug 22, 2024
View reviewed changes
docker-bake.hcl Outdated Show resolved Hide resolved
@elkoniu elkoniu force-pushed the add-misisng-python-lib branch 9 times, most recently from a0d410d to 9392d51 Compare August 27, 2024 09:36
achilleas-k
achilleas-k approved these changes Aug 27, 2024
View reviewed changes
Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

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

Messy but as long as it works I suppose it LGTM :)

@thozza
Copy link
Member

thozza commented Aug 27, 2024

It does not work 😢

#10 [target 4/8] RUN             ./src/scripts/dnf.sh "bash
bubblewrap
coreutils
cryptsetup
curl
dnf
dnf-plugins-core
dosfstools
e2fsprogs
findutils
git
glibc
iproute
lvm2
make
ostree
policycoreutils
python-rpm-macros
python3
python3-devel
python3-iniparse
python3-jsonschema
python3-librepo
python3-mako
python3-pip
python3-pyyaml
python3-rpm-generators
python3-rpm-macros
qemu-img
rpm
rpm-build
rpm-ostree
rpmdevtools
skopeo
systemd
systemd-container
tar
util-linux
fuse-overlayfs
 python3-tomli-w" "development tools,rpm-development-tools" 1

@elkoniu elkoniu force-pushed the add-misisng-python-lib branch 9 times, most recently from 79090bf to 10d3b39 Compare August 30, 2024 08:56
@elkoniu
Copy link
Contributor Author

elkoniu commented Aug 30, 2024

The current state is that python3-tomli-w has been recently removed from c10s images and we need to find replacement for it.

@elkoniu elkoniu force-pushed the add-misisng-python-lib branch from 10d3b39 to 99413b7 Compare September 11, 2024 16:05
@elkoniu
Copy link
Contributor Author

elkoniu commented Sep 11, 2024

@thozza the status of this PR is that I extended c9s with tomli package and c10s is unchanged. Still - mechanics I have added allows to add distro specific packages in the future and I think it may be worth keeping. What is your opinion about that?

thozza
thozza reviewed Sep 11, 2024
View reviewed changes
docker-bake.hcl
Comment on lines -363 to -366
//"python3-autopep8", // not available in cXs
//"python3-boto3", // not available in cXs
//"python3-botocore", // not available in cXs
//"python3-docutils", // not available in cXs
Copy link
Member

Choose a reason for hiding this comment

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

So, the intention with the commented out items was to keep the list the same with Fedora container, with the packages that are not available in cXs commented out with an explanation. Because it may be confusing to have some tools installed as an RPM package in Fedora, but using pip in centos.

If not a big deal, I would keep it that way.

Copy link
Member

Choose a reason for hiding this comment

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

We probably can't keep comments with the new list, right? 🤔

Copy link
Contributor Author

@elkoniu elkoniu Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Let me describe the whole story:)
For easier modification / review it is the best to keep this as a multi line string (as before). The problem is that bake syntax allows multiline string in here-doc format only. The other problem is that it was impossible in bake to append inherited variable from base image, so the only way to have this common module list was to put it as a separate variable.

There are three possible solutions, each one not perfect:

  1. I will add all the comments on top of the new variable but they will no longer be inline comments
  2. I could add them inline and modify dnf script processing it later to filter them out when it is processing list of packages.
  3. Copy paste the list with comments to each image definition with it own comments (which kills the whole inheritance idea).

I would opt for option (1) as I agree it is worth to keep those comments

@elkoniu elkoniu force-pushed the add-misisng-python-lib branch from 99413b7 to b05c2f6 Compare September 13, 2024 09:48
@elkoniu elkoniu mentioned this pull request Sep 13, 2024
Merged
@thozza thozza closed this Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thozza thozza thozza left review comments

@achilleas-k achilleas-k achilleas-k approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@elkoniu @thozza @achilleas-k