-
Notifications
You must be signed in to change notification settings - Fork 321
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
Support for Testing Python container by container-ci-suite #675
base: master
Are you sure you want to change the base?
Conversation
…ft 4 Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this is just a draft so I did just a very quick review.
@@ -0,0 +1,73 @@ | |||
import os | |||
import sys | |||
import pytest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you split imports, there are usually three groups: standard library, third-party libs (installed dependencies) and local modules.
|
||
from container_ci_suite.openshift import OpenShiftAPI | ||
from container_ci_suite.utils import get_service_image, check_variables | ||
test_dir = Path(os.path.abspath(os.path.dirname(__file__))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You use all upper case names for constant and this is also constant.
IMAGE_TAG = "postgresql:10" | ||
PSQL_VERSION = "10" | ||
|
||
if OS == "rhel7": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A dictionary (mapping) might be better than many ifs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
test_dir = Path(os.path.abspath(os.path.dirname(__file__))) | ||
|
||
if not check_variables(): | ||
sys.exit(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about some error message here is the exit code is non-zero?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The messages are here: https://github.com/sclorg/container-ci-suite/blob/main/container_ci_suite/utils.py#L242.
This pull request uses testing s2i-python-container in OpenShift 4 environment
by container-ci-suite.