Skip to content

Conversation

mvk
Copy link
Contributor

@mvk mvk commented Sep 4, 2025

changes:

  • update the list of packages:
    • del: requests, paramiko (they get installed as dependencies)
    • add: jsonpatch (missing package)
  • update Containerfile:
    • do COPY before pip install

tests:

1. ensure pip3 freeze changes as expected:

cmd="pip3 freeze"
for br in main requirements-file; do
    img="localhost/eco-ci-cd:br-${br}"
    git switch "${br}"
    podman build --platform linux/amd64 -t "${img}" -f Containerfile .
    podman run --platform linux/amd64 --rm -it "${img}" -c "${cmd}" > ../"pip.freeze.${br}.txt"
done

diff -u ../pip.freeze.{main,requirements-file}.txt
--- ../pip.freeze.main.txt      2025-09-05 00:08:16.698870929 +0300
+++ ../pip.freeze.requirements-file.txt 2025-09-05 00:08:17.299602169 +0300
@@ -22,6 +22,8 @@
 Jinja2==3.1.6
 jira==3.8.0
 jmespath==1.0.1
+jsonpatch==1.33
+jsonpointer==3.0.0
 jsonschema==4.25.1
 jsonschema-specifications==2025.4.1
 junitparser==4.0.2

2. ensure adding a line in requirements.txt updates a layer (modifies the layer):

br=requirements-file
img=localhost/eco-ci-cd:br-${br}-xyz"
echo "argcomplete" >> requirements.txt
podman build --platform linux/amd64 -t "${img}" -f Containerfile .
podman image inspect  localhost/eco-ci-cd:br-requirements-file  | jq -r '.[].RootFS.Layers[]' > ../layers.txt
podman image inspect  localhost/eco-ci-cd:br-requirements-file-xyz  | jq -r '.[].RootFS.Layers[]' > ../layers.xyz.txt

diff -u ../layers.txt ../layers.xyz.txt
--- ../layers.txt       2025-09-05 00:19:24.897637066 +0300
+++ ../layers.xyz.txt   2025-09-05 00:19:41.251512540 +0300
@@ -1,5 +1,5 @@
sha256:b989f0b527ca3362fbfd6c387a16da6e8f5d56d8d850fd3562b774d8cd84e819
sha256:bc484743fcedc50c4f13cbd85051cbefd88c96d93fb35aaf106be58f35629662
-sha256:f1b2768a10521193173269cd6ac265026d0439696f56a8311b3d04eb0886e02f
-sha256:618b07f709daacec49248d3f8ee79f3857298b58c13b1a1b95fe8687d4b72551
-sha256:4cb50120280d9f0446f95db36ebe5a626aab200453fe4a5f2c12bca93ea7e5b7
+sha256:c69646b459101f6db970162562d98b86568a749fb8a33607d6f3e259bfe04b02
+sha256:7b2df91c58cefbbdf231bb3ce89c94d106389e2454b38975d4401fb21d097dbf
+sha256:845145bdb5cc19b8afca6114bfc1abab81ec2adfce720660c70f255ab34b6002

as expected.

@mvk
Copy link
Contributor Author

mvk commented Sep 7, 2025

/assign @eifrach

@eifrach I have tested the container layer effects of changing requirements.txt.

  1. adding new package to requirements.txt => does trigger the pip install layer to be re-created (no regression in missing packages that have to be installed)
  2. adding no package change, just e.g. adding comment line in requirements.txt also triggers the layer to be rebuilt, unfortunately, But this happens in both use cases: with requirements.txt AND explicit line in the Containerfile (so no regression here too)

Containerfile Outdated
Comment on lines 13 to 18
# Copy application files to eco-ci-cd folder
COPY . .

# Install ansible and ansible-lint
RUN pip3 install --no-cache-dir -r requirements.txt

Copy link
Contributor

Choose a reason for hiding this comment

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

so to improve layring it's better to install the requirement before coping the code
this will insure that change of code will not re-run the requirement layer

Suggested change
# Copy application files to eco-ci-cd folder
COPY . .
# Install ansible and ansible-lint
RUN pip3 install --no-cache-dir -r requirements.txt
# Copy requirements file
COPY requirements.txt .
# Install ansible and ansible-lint
RUN pip3 install --no-cache-dir -r requirements.txt
# Copy application files to eco-ci-cd folder
COPY . .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eifrach good point 👍
do you like it better now ?

Copy link
Contributor

@eifrach eifrach left a comment

Choose a reason for hiding this comment

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

approved with a note

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 7, 2025
Copy link

openshift-ci bot commented Sep 7, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eifrach

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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 7, 2025
@eifrach
Copy link
Contributor

eifrach commented Sep 7, 2025

/hold
remove the hold if you don't want the changes

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 7, 2025
@mvk mvk force-pushed the requirements-file branch from 4b13df6 to 352b1d2 Compare September 7, 2025 10:40
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 7, 2025
- update the list of packages:
    - del: `requests`, `paramiko` (installed as dependencies)
    - add: `jsonpatch` (missing package)
- pip requirements in `pip.txt`
- update `Containerfile`:
    - do `COPY` of `pip.txt` and `requirements.yml` before their install

Signed-off-by: Maxim Kovgan <[email protected]>
@mvk mvk force-pushed the requirements-file branch from 352b1d2 to 9ce7355 Compare September 7, 2025 10:53
@eifrach
Copy link
Contributor

eifrach commented Sep 7, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 7, 2025
@eifrach
Copy link
Contributor

eifrach commented Sep 7, 2025

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 7, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit a33fc41 into openshift-kni:main Sep 7, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants