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

Replace requests with urllib in prune_images script #88

Merged
merged 6 commits into from
Jan 12, 2024

Conversation

tkdchen
Copy link
Contributor

@tkdchen tkdchen commented Jan 5, 2024

STONEBLD-1785

This replacement ensures no write operation to the rootFS happens during the job execution.

Major changes:

  • Part of the prune_images script is rewritten with urllib instead of requests.
  • Test code is also rewritten so that prune_images script uses all builtin libraries for execution and development. No write operation is required for running the script during the cronjob execution.
  • Additional tests are added.

Run tests:

cd config/registry_image_pruner/image_pruner/
python3.9 test_prune_images.py

In addition, two small refactors are made and a GitHub workflow action is added to run the tests in CI.

Copy link

openshift-ci bot commented Jan 5, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@tkdchen tkdchen force-pushed the STONEBLD-1785-pruner branch 2 times, most recently from 52dab51 to bcf330f Compare January 8, 2024 03:26
STONEBLD-1785

* Part of the prune_images script is rewritten with urllib instead of
  requests.
* Test code is also rewritten so that prune_images script uses all
  builtin libraries for execution and development. No write operation is
  required for running the script during the cronjob execution.
* Additional tests are added.

Run tests:

  cd config/registry_image_pruner/image_pruner/
  python3.9 test_prune_images.py

Signed-off-by: Chenxiong Qi <[email protected]>
This makes mypy happy.

Signed-off-by: Chenxiong Qi <[email protected]>
@tkdchen tkdchen marked this pull request as ready for review January 8, 2024 03:42
@openshift-ci openshift-ci bot requested review from mmorhun and psturc January 8, 2024 03:42
@tkdchen tkdchen requested review from tisutisu, lkolacek, rcerven, mkosiarc and chmeliik and removed request for psturc January 8, 2024 03:42
@tkdchen tkdchen changed the title Stonebld 1785 pruner Replace requests with urllib in prune_images script Jan 8, 2024
@tkdchen tkdchen mentioned this pull request Jan 8, 2024
config/registry_image_pruner/cronjob.yaml Show resolved Hide resolved
config/registry_image_pruner/image_pruner/prune_images.py Outdated Show resolved Hide resolved
Comment on lines +95 to +97
if not repos:
LOGGER.debug("No image repository is found.")
break
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to break when there are no repositories? It looks like the script previously didn't do that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The script didn't do that previously. It could call the process_repositories with an empty list of repos or None. I don't want that. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I previously chose that approach because it felt more pythonic to me
as the first line of the function

for repo in repos:

would just ignore empty repos. This way we have to have a condition and a break, but if the images can really be None then this approach make sense

@mkosiarc
Copy link
Contributor

mkosiarc commented Jan 8, 2024

I know you already did the work, but I would have prefered if we just had an image with the requests library in our new dockerfiles repos

These separate unit tests are based on the previous tests.

Signed-off-by: Chenxiong Qi <[email protected]>
@tkdchen tkdchen requested a review from chmeliik January 10, 2024 09:51
Copy link
Contributor

@chmeliik chmeliik left a comment

Choose a reason for hiding this comment

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

LGTM 👍

config/registry_image_pruner/image_pruner/prune_images.py Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm label Jan 11, 2024
* Address review comment by replacing name image with tag.
* Remove regex compilation flag to avoid confusion. Previous regex match
  did not use it.
* Remove unused import.

Signed-off-by: Chenxiong Qi <[email protected]>
Copy link

sonarcloud bot commented Jan 11, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@openshift-ci openshift-ci bot added the lgtm label Jan 11, 2024
@tkdchen tkdchen merged commit 5778ca2 into konflux-ci:main Jan 12, 2024
18 checks passed
@tkdchen tkdchen deleted the STONEBLD-1785-pruner branch January 12, 2024 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants