-
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
add test for container push on Katello 4.14+ #1853
Conversation
CONTAINER_PULL_LABEL=$(echo "${ORGANIZATION_LABEL}-${PRODUCT_LABEL}-${CONTAINER_REPOSITORY_LABEL}"| tr '[:upper:]' '[:lower:]') | ||
CONTAINER_PUSH_LABEL=$(echo "${ORGANIZATION_LABEL}/${PRODUCT_LABEL}/${CONTAINER_REPOSITORY_LABEL}-bats-$(date -u '+%s')"| 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.
Yes, you need to pull with ORG-PRODUCT-REPO
but push with ORG/PRODUCT/REPO
.
@ianballou this hurts 😿
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.
Aye it does - @ianballou is looking into whether we could switch to a /
based naming scheme which is more common within the container world.
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.
interestingly, pushed stuff is then also only pull-able with /
-- so -
only applies to synced stuff? confusing!
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.
Exactly, we wanted content to be pullable with the same path that you pushed to. The way synced content gets distributed is out of date and confusing for sure.
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.
Synced content follows a naming scheme that the user can set on a per-environment basis. I guess in the early container days you couldn't have slashes, because dashes were chosen for some reason for the default.
Jump forward to now with container push, if we kept the environmental naming pattern, then users would see names change after they push, which is bad. So, for push, the naming scheme set on environments is ignored.
Now all that's left is the correct the old pattern for synced repositories.
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 will add -- if you want all slashes before we fix it, BATS could change the container naming scheme :)
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.
nah, let's keep as close to the default config as possible
bats/foreman_helper.bash
Outdated
@@ -53,6 +53,13 @@ tSkipIfNewerThan45() { | |||
fi | |||
} | |||
|
|||
tContainerPushSupported() { | |||
KATELLO_VERSION=$(tKatelloVersion) | |||
if ! tIsVersionNewer "${KATELLO_VERSION}" 4.13; then |
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.
That's based on Katello/katello#10952 -- is that a correct assumption?
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.
That PR is just the beginning, I need to check but I think 4.14 would be 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.
@evgeni I confirmed that Katello 4.14 and higher should be used here.
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.
done!
a45c656
to
b3c1a67
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.
This looks fine at a glance, was it tested on a live system?
I did run it once on nightly and it didn't totally freak out, yeah :) |
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.
Looks like there was just enough of the container push feature implemented for this test to work in 4.13, nice. Ack!
I confirmed the feature wasn't fully done for Katello 4.13 -- can we change the requirement to 4.14+ ?
Saw it was only tested on nightly, oops
bats/foreman_helper.bash
Outdated
@@ -53,6 +53,13 @@ tSkipIfNewerThan45() { | |||
fi | |||
} | |||
|
|||
tContainerPushSupported() { | |||
KATELLO_VERSION=$(tKatelloVersion) | |||
if ! tIsVersionNewer "${KATELLO_VERSION}" 4.13; then |
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.
@evgeni I confirmed that Katello 4.14 and higher should be used here.
b3c1a67
to
9a7fd65
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.
Looks good!
No description provided.