-
Notifications
You must be signed in to change notification settings - Fork 200
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
Use newer container naming in container BATS #1881
Use newer container naming in container BATS #1881
Conversation
bats/fb-katello-container.bats
Outdated
tContainerSlashesDefault | ||
tPackageExists podman || tPackageInstall podman | ||
podman login "${HOSTNAME}" -u admin -p changeme | ||
CONTAINER_PULL_LABEL=$(echo "${ORGANIZATION_LABEL}/${PRODUCT_LABEL}/${CONTAINER_REPOSITORY_LABEL}"| tr '[:upper:]' '[:lower:]') |
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.
Instead of adding a new test, should we guard this variable behind a version check? Something like
CONTAINER_PULL_LABEL=$(echo "${ORGANIZATION_LABEL}/${PRODUCT_LABEL}/${CONTAINER_REPOSITORY_LABEL}"| tr '[:upper:]' '[:lower:]') | |
if tIsVersionNewer "${KATELLO_VERSION}" 4.16 | |
CONTAINER_PULL_LABEL=$(echo "${ORGANIZATION_LABEL}/${PRODUCT_LABEL}/${CONTAINER_REPOSITORY_LABEL}"| tr '[:upper:]' '[:lower:]') | |
else | |
CONTAINER_PULL_LABEL=$(echo "${ORGANIZATION_LABEL}-${PRODUCT_LABEL}-${CONTAINER_REPOSITORY_LABEL}"| tr '[:upper:]' '[:lower:]') | |
fi |
My reasoning is you always only run one or the other. If there was a version where you ran both tests then an additional test is obviously better.
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.
Sure, that works for me.
81184e2
to
66b06c2
Compare
66b06c2
to
5d69ef3
Compare
5d69ef3
to
516cb02
Compare
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 haven't been able to actually test this yet due to issues running the pipeline locally.
I trust you on it. Do you want to merge this now?
Merging! It'll be broken tomorrow regardless if we don't merge this, so at least it should get us closer even if there's an issue here. |
I just ran a nightly pipeline and getting a failure:
Note this is in a different file: forklift/bats/fb-katello-client-global-registration.bats Lines 77 to 82 in 518b53b
But also in the container file it's using dashes, not slashes. I can't quite see what the value of |
We have container tests outside the container test file? Dang... Well I'll need to try it myself, I should be able to get the pipeline running locally if you did. |
In the global registration I think it's verifying that it sets up podman auth so you can pull container images. But note that also the tests you modified don't pass. Really looks like either the version is wrong or the comparison.
I'll admit I just relied on Jenkins and I only copied the results from there. It's been a while since I ran it locally. |
I think I'll just remove the podman test, we don't have registration automation for setting up container registry authentication (yet :) )
Yeah, it was just wishful thinking assuming that I'd get those comparisons right the first try. I'm aiming to get it settled today. |
I have some vague recollection we set up certificates for podman somewhere so it could pull, but I can't find that now. |
I suppose testing that some other system can pull container content is worthwhile. The CA does need to be installed so podman can trust the repo. The tests on the nightly box itself wouldn't catch issues there. |
Since container repos now have slashes in them, the test needs updating.
I also updated the version requirement for the container push test since it should run on Katello 4.14. (It still needs investigation though :) )
I haven't been able to actually test this yet due to issues running the pipeline locally.