search

thoth-station / s2i-thoth

Thoth's addition to OpenShift's s2i Python builds to benefit from Thoth's recommendations in your application
GNU General Public License v3.0
6 stars 19 forks source link

adding required files for f35-py310 S2I image to built #235

Closed Gregory-Pereira closed 2 years ago

Gregory-Pereira commented 2 years ago

Related Issues and Dependencies

Related to https://github.com/thoth-station/s2i-thoth/issues/231

Includes linting changes for all s2i_assemble.patch files, which is outside this PR's scope, but otherwise pre-commit check won't pass. Additionally added documentation to README.rst so people don't try to manually build the patch like I did...

This introduces a breaking change

This Pull Request implements

Implements the files required to build a Thoth image of fedora35 with python3.10 using S2I.

Description

Solely creates the base image, a solver image still needs to be built from this.

goern commented 2 years ago

/lgtm /assign @Gregory-Pereira

sesheta commented 2 years ago

New changes are detected. LGTM label has been removed.

Gregory-Pereira commented 2 years ago

nice work. was the s2i_assemble.patch copied from other folders? and another minor question is changes to ubi file intentional ?

  1. Yes I copied the file for the s2i_assemble.patch living in the f34-py39 folder, and went to change anything specific to fedora version 34 or python version 3.9, but there were none. I did this because I wasn't exactly sure how these were meant to be implemented / generated, if there is a better way I am all ears.

  2. I did not want to change the UBI assemble patches however, there was some formatting issue in them that made the pre-commit fail. I am guessing it was something in this commit that caused it, and I am not sure if changes were forced into master, the pre-commit was down, or the configuration file for precommit changed since then.

harshad16 commented 2 years ago
  1. Yes I copied the file for the s2i_assemble.patch living in the f34-py39 folder, and went to change anything specific to fedora version 34 or python version 3.9, but there were none. I did this because I wasn't exactly sure how these were meant to be implemented / generated, if there is a better way I am all ears.

The reason why I asked this is because, the s2i_assemble.patch is created based on the difference of the assemble present in the base image and addition we are trying to do from our assemble script. As the assemble bit can differ from base image to base image, it is good to verify this once.

*. Easiest way to verify: Try to build the image from the dockerfile. (if successful the patch is in good shape)

state the copied assemble script in a/assemble state the removed bit assemble script in b/assemble diff -u a/assemble b/assemble > assemble.patch

the resultant patch is what we require.

  1. I did not want to change the UBI assemble patches however, there was some formatting issue in them that made the pre-commit fail. I am guessing it was something in this commit that caused it, and I am not sure if changes were forced into master, the pre-commit was down, or the configuration file for precommit changed since then.

Thanks for the changes :)

harshad16 commented 2 years ago

/hold

hold till the image build is confirmed.

harshad16 commented 2 years ago

The s2i_assemble.patch script fails with f35 base image to be patched:

/usr/libexec/s2i ~
File assemble is read-only; trying to patch anyway
patching file assemble
Hunk #2 FAILED at 23.
1 out of 2 hunks FAILED -- saving rejects to file assemble.rej

please try to create the s2i_assemble.patch as instructed above if facing any issue, let me know

Gregory-Pereira commented 2 years ago

Got it, thanks Harshad, taking a look now.

Gregory-Pereira commented 2 years ago

Can confirm that the f35-py310 overlay now builds correctly:

$ podman build .
STEP 1/8: FROM registry.fedoraproject.org/f35/python3:0-41.container
...
STEP 8/8: USER 1001
--> Using cache 83dc730284d93fd5378c557bf4a3f1fc58647b0f8279eaab5aeb147de067fc78
--> 83dc730284d
83dc730284d93fd5378c557bf4a3f1fc58647b0f8279eaab5aeb147de067fc78
Gregory-Pereira commented 2 years ago

/hold Working on formatting the README.rst file.

Gregory-Pereira commented 2 years ago

/unhold Ready for review. To preview formatted doc changes, see my fork

sesheta commented 2 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fridex

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/thoth-station/s2i-thoth/blob/master/OWNERS)~~ [fridex] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment