-
Notifications
You must be signed in to change notification settings - Fork 69
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
Production file-based catalog specific for OCP versions #2926
Production file-based catalog specific for OCP versions #2926
Conversation
The build error is cause by the fact that two versions replace the same previous version:
|
6a4c17d
to
18ba81e
Compare
Getting this error in the github action:
Probably we don't have the right credentials yet. |
.github/workflows/validate.yaml
Outdated
env: | ||
REGISTRY_REDHAT_IO_USERNAME: ${{ secrets.REGISTRY_REDHAT_IO_USERNAME }} | ||
REGISTRY_REDHAT_IO_PASSWORD: ${{ secrets.REGISTRY_REDHAT_IO_PASSWORD }} | ||
run: make generated-files generate-catalog |
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'd include generate-catalog
in generated-files
so that others don't need to learn about this new command to run ?
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 command "generate-catalog" takes a looot of time. About 1/2 hour on my localhost. Maybe it will be faster in the github action but let's see.
The other problem is that it requires credentials to registry.redhat.io and those are not available in github actions that run "on-pull-request".
I'm going to push a little change to split this. We could run "generate-catalog" only periodically or on-push.
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.
Pushed the split.
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.
wow 1/2 hour
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.
Alright. In github action it's much faster. About 5 minutes:
11:28:54.752 INFO: Generating catalog for OCP 4.13
time="2024-10-11T11:28:54Z" level=info msg="rendering index \"registry.redhat.io/redhat/redhat-operator-index:v4.13\" as file-based catalog"
time="2024-10-11T11:29:49Z" level=info msg="wrote rendered file-based catalog to \"/tmp/knative.aIjkdvzz/tmp.IiMQGlnAOi\"\n"
11:30:27.698 INFO: Generating catalog for OCP 4.14
time="2024-10-11T11:30:27Z" level=info msg="rendering index \"registry.redhat.io/redhat/redhat-operator-index:v4.14\" as file-based catalog"
time="2024-10-11T11:31:21Z" level=info msg="wrote rendered file-based catalog to \"/tmp/knative.aIjkdvzz/tmp.s4bv1h7HZi\"\n"
11:31:39.478 INFO: Generating catalog for OCP 4.15
time="2024-10-11T11:31:39Z" level=info msg="rendering index \"registry.redhat.io/redhat/redhat-operator-index:v4.15\" as file-based catalog"
time="2024-10-11T11:32:31Z" level=info msg="wrote rendered file-based catalog to \"/tmp/knative.aIjkdvzz/tmp.vOKEBnWFRH\"\n"
11:32:48.077 INFO: Generating catalog for OCP 4.16
time="2024-10-11T11:32:48Z" level=info msg="rendering index \"registry.redhat.io/redhat/redhat-operator-index:v4.16\" as file-based catalog"
time="2024-10-11T11:33:36Z" level=info msg="wrote rendered file-based catalog to \"/tmp/knative.aIjkdvzz/tmp.mKSK5ppXVI\"\n"
11:33:57.680 INFO: Generating catalog for OCP 4.17
time="2024-10-11T11:33:57Z" level=info msg="rendering index \"registry.redhat.io/redhat/redhat-operator-index:v4.17\" as file-based catalog"
time="2024-10-11T11:34:12Z" level=info msg="wrote rendered file-based catalog to \"/tmp/knative.aIjkdvzz/tmp.TB6GDPKkiP\"\n"
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.
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.
and I guess the same for others, but if that's normal, nevermind my noob questions
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.
In the same file I see old CI images
{
"name": "knative-openshift-ingress",
"image": "registry.ci.openshift.org/knative/release-1.34.0:serverless-ingress"
},
{
"name": "knative-openshift",
"image": "registry.ci.openshift.org/knative/release-1.34.0:serverless-knative-operator"
},
{
"name": "knative-operator",
"image": "registry.ci.openshift.org/knative/release-1.34.0:serverless-openshift-knative-operator"
},
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.
shouldn't there be prod images only in this file as that's part of the final release?
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 got the same, when I created them as described https://github.com/konflux-ci/olm-operator-konflux-sample/blob/main/docs/konflux-onboarding.md#create-the-fbc-in-the-git-repository
btw: those commands also offer a --output yaml
parameter. So the catalog-templat might be easier to maintain later.
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 that comment ? #2926 (comment)
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.
is it correct / what we expect ?
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 guess it's here the reply
This will be automatically fixed later. When 1.34 is pushed to the production catalog it will be pulled when calling "generate-catalog" and the script will skip this version and won't add CI images there.
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. That's the answer.
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.
Pushed the yaml variant ! I like it much more.
hack/lib/images.bash
Outdated
function latest_registry_ci_sha() { | ||
input=${1:?"Provide image"} | ||
|
||
image_without_tag=${input%:*} # Remove tag, if any | ||
|
||
go_bin="$(go env GOPATH)/bin" | ||
export GOPATH="$PATH:$go_bin" | ||
digest=$(skopeo inspect --no-tags=true "docker://${input}" | jq -r '.Digest') | ||
if [ "${digest}" = "" ]; then | ||
exit 1 | ||
fi | ||
|
||
echo "${image_without_tag}@${digest}" | ||
} |
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 think we could reuse latest_konflux_image_sha
for this ?
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 check this. We don't want the "latest" tag though. We want a specific tag, e.g. release-1.34.0. The latest_konflux_image_sha
automatically adds "latest"
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.
Pushed.
hack/lib/images.bash
Outdated
image_without_tag=${input%:*} # Remove tag, if any | ||
|
||
go_bin="$(go env GOPATH)/bin" | ||
export GOPATH="$PATH:$go_bin" |
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.
not sure, why this is needed?
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'm not sure. I copied it from the other functions in this file. Looks like it works without this as well. Maybe @pierDipi knows?
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.
Removed the line in the new commit.
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.
not everyone has the go/bin folder in their path and go install
installs binaries in go/bin but I'm ok making it a prerequisite
59eda4d
to
4783016
Compare
Awesome! |
Awesome 🎉 |
/lgtm |
1 similar comment
4783016
to
c2e5e66
Compare
I had to do a rebase because of some other changes so I added a couple of commits and fixed the above as well. |
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: creydr, mgencur, pierDipi 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 |
/test 416-test-upgrade-aws-416 |
51a4642
into
openshift-knative:main
https://issues.redhat.com/browse/SRVCOM-3369
Proposed Changes