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

Reduce python APIv2 test net dependency #23538

Merged

Conversation

cevich
Copy link
Member

@cevich cevich commented Aug 7, 2024

Previously these tests pulled some test images from quay, opening them up to networking-flake induced failures. As has already been done for other tests, update to utilize the locally running registry server.

Also: Add test/python/** into the apiv2 task conditions as referenced
by the Makefile localapiv2-python target.


This is a guess at fixing flakes like https://api.cirrus-ci.com/v1/artifact/task/6485548934627328/html/apiv2-podman-fedora-40-root-host-sqlite.log.html

But also an update these tests should receive regardless.

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added release-note-none do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Aug 7, 2024
@cevich
Copy link
Member Author

cevich commented Aug 7, 2024

Note to me: .cirrus.yml is broken, the apiv2_test_task doesn't match changes in test/python.

@cevich cevich force-pushed the apiv2_python_use_local_reg branch 2 times, most recently from d2f757d to 8cd4172 Compare August 7, 2024 19:40
@Luap99
Copy link
Member

Luap99 commented Aug 8, 2024

Note to me: .cirrus.yml is broken, the apiv2_test_task doesn't match changes in test/python.

It looks to me we are running the docker-py tests (test/python/docker) twice. Once as separate task docker-py compat
make run-docker-py-tests and it also part of apiv2 test via make localapiv2-python.
As such let's just drop the docker-py tasks and fix the cirrus source skip conditions on the apiv2 task to include test/python

.cirrus.yml Outdated Show resolved Hide resolved

with open(os.path.join(reg_conf_source_path)) as file:
conf = file.read() + reg_conf_sfx

Copy link
Member

Choose a reason for hiding this comment

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

if you feel like it below there is a bunch of CNI references that are not used at all anymore and could be dropped

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, ya I was scratching my head on that. Maybe it's best done as a followup PR.

Copy link
Member

Choose a reason for hiding this comment

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

agree, I found some other cni references as well so if I find some free time I might get around to nuke that stuff finally.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh okay, thanks. I started on #23552 but it can't move forward until this merges (b/c of changesInclude() updates). I have no close attachment to that PR if you have a mind to take on this effort, otherwise I'll continue as best I can after this merges.

@cevich cevich force-pushed the apiv2_python_use_local_reg branch from 8cd4172 to 6bb713b Compare August 8, 2024 14:28
cevich added 2 commits August 8, 2024 10:40
Previously, if anyone touched these files no extra testing would
trigger.  However, basically all testing depends on them.  Update the
condition and test that verifies it.

Signed-off-by: Chris Evich <[email protected]>
Previously these tests pulled some test images from quay, opening them up
to networking-flake induced failures.  As has already been done for
other tests, update to utilize the locally running registry server.

Also: Add `test/python/**` into the apiv2 task conditions as referenced
by the `Makefile` `localapiv2-python` target.

Signed-off-by: Chris Evich <[email protected]>
@cevich cevich force-pushed the apiv2_python_use_local_reg branch from 6bb713b to 7936809 Compare August 8, 2024 14:40
@cevich cevich marked this pull request as ready for review August 8, 2024 15:31
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 8, 2024
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

It looks to me we are running the docker-py tests (test/python/docker) twice. Once as separate task docker-py compat
make run-docker-py-tests and it also part of apiv2 test via make localapiv2-python.

This point is still open, I think we should just drop the docker-py task but that can happen in a another PR.

Copy link
Contributor

openshift-ci bot commented Aug 8, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cevich, Luap99

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 Aug 8, 2024
@cevich
Copy link
Member Author

cevich commented Aug 8, 2024

It looks to me we are running the docker-py tests (test/python/docker) twice.

I think they're different:

.PHONY: localapiv2-python
localapiv2-python:
	env CONTAINERS_CONF=$(CURDIR)/test/apiv2/containers.conf PODMAN=./bin/podman \
		pytest --verbose --disable-warnings ./test/apiv2/python
	touch test/__init__.py
	env CONTAINERS_CONF=$(CURDIR)/test/apiv2/containers.conf PODMAN=./bin/podman \
		pytest --verbose --disable-warnings ./test/python/docker
	rm -f test/__init__.py

One is testing the REST API the other is testing the interface used by docker-py IIRC. It's been a really long time since I worked on this setup w/ Jhon. Maybe he remembers better?

@rhatdan
Copy link
Member

rhatdan commented Aug 8, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 8, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit d13f2a5 into containers:main Aug 8, 2024
91 checks passed
@Luap99
Copy link
Member

Luap99 commented Aug 9, 2024

It looks to me we are running the docker-py tests (test/python/docker) twice.

I think they're different:

.PHONY: localapiv2-python
localapiv2-python:
	env CONTAINERS_CONF=$(CURDIR)/test/apiv2/containers.conf PODMAN=./bin/podman \
		pytest --verbose --disable-warnings ./test/apiv2/python
	touch test/__init__.py
	env CONTAINERS_CONF=$(CURDIR)/test/apiv2/containers.conf PODMAN=./bin/podman \
		pytest --verbose --disable-warnings ./test/python/docker
	rm -f test/__init__.py

One is testing the REST API the other is testing the interface used by docker-py IIRC. It's been a really long time since I worked on this setup w/ Jhon. Maybe he remembers better?

They are identical calls other than the versbose option which should not make difference

.PHONY: run-docker-py-tests
run-docker-py-tests:
	touch test/__init__.py
	env CONTAINERS_CONF=$(CURDIR)/test/apiv2/containers.conf pytest --disable-warnings test/python/docker/
	rm -f test/__init__.py

@cevich
Copy link
Member Author

cevich commented Aug 9, 2024

run-docker-py-tests:

Ooohhhh, that's the one you were referring to. I thought it was the two items under localapiv2-python. My eyes glazed over seeing docker-py so much 🤣 Ya, we should delete that one, I'll open a PR.

@cevich
Copy link
Member Author

cevich commented Aug 9, 2024

#23562

@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Nov 8, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Nov 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants