search

openshift / cluster-samples-operator

The samples operator installs+maintains the sample templates+imagestreams on a cluster
29 stars 89 forks source link

OKD-225: Add back mysql samples #571

Closed ausil closed 1 month ago

ausil commented 2 months ago

It turns out that there is a test for particular samples being imported into the imagestreams https://github.com/openshift/origin/blob/master/test/extended/util/framework.go#L315 is the list. so lets add back mysql even though it is unsupported

openshift-ci-robot commented 2 months ago

@ausil: This pull request references OKD-225 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.18.0" version, but no target version was set.

In response to [this](https://github.com/openshift/cluster-samples-operator/pull/571): >It turns out that there is a test for particular samples being imported into the imagestreams >https://github.com/openshift/origin/blob/master/test/extended/util/framework.go#L315 is the list. so lets add back mysql even though it is unsupported Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fcluster-samples-operator). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
Prashanth684 commented 2 months ago

only touches okd so should be safe for merging

Prashanth684 commented 2 months ago

/lgtm

ausil commented 2 months ago

/assign mfrancisc

metlos commented 1 month ago

The failing image-ecosystem test is being fixed in openshift/origin#29116. But I wonder what breakage is https://github.com/openshift/origin/blob/master/test/extended/util/framework.go#L315 causing? From the OCP perspective, I'd much rather fix that test than bring additional samples back to support.

Prashanth684 commented 1 month ago

The failing image-ecosystem test is being fixed in openshift/origin#29116. But I wonder what breakage is https://github.com/openshift/origin/blob/master/test/extended/util/framework.go#L315 causing? From the OCP perspective, I'd much rather fix that test than bring additional samples back to support.

We add MySQL back to fix the breakage of that test or we remove MySQL from https://github.com/openshift/origin/blob/master/test/extended/util/framework.go#L315. @metlos are you suggesting removing MySQL from that test?

metlos commented 1 month ago

The failing image-ecosystem test is being fixed in openshift/origin#29116. But I wonder what breakage is https://github.com/openshift/origin/blob/master/test/extended/util/framework.go#L315 causing? From the OCP perspective, I'd much rather fix that test than bring additional samples back to support.

We add MySQL back to fix the breakage of that test or we remove MySQL from https://github.com/openshift/origin/blob/master/test/extended/util/framework.go#L315. @metlos are you suggesting removing MySQL from that test?

Well, even though some of the imagestreams in that test are not supported, we keep them in the assets, even though we don't update them.

So at a glance, I didn't see what breakage that test might be causing. But I assumed I didn't look deep enough in the code so I asked what breakage is being fixed by updating the unsupported image streams.

The test doesn't need to be removed, just updated as soon as we "loose" some of the image streams for good (which IMHO didn't happen yet). Loosing the image stream would only happen if we removed all its versions due to EOL.

Prashanth684 commented 1 month ago

The failing image-ecosystem test is being fixed in openshift/origin#29116. But I wonder what breakage is https://github.com/openshift/origin/blob/master/test/extended/util/framework.go#L315 causing? From the OCP perspective, I'd much rather fix that test than bring additional samples back to support.

We add MySQL back to fix the breakage of that test or we remove MySQL from https://github.com/openshift/origin/blob/master/test/extended/util/framework.go#L315. @metlos are you suggesting removing MySQL from that test?

Well, even though some of the imagestreams in that test are not supported, we keep them in the assets, even though we don't update them.

So at a glance, I didn't see what breakage that test might be causing. But I assumed I didn't look deep enough in the code so I asked what breakage is being fixed by updating the unsupported image streams.

The test doesn't need to be removed, just updated as soon as we "loose" some of the image streams for good (which IMHO didn't happen yet). Loosing the image stream would only happen if we removed all its versions due to EOL.

Right. So, we are adding MySQL back because the e2e test checks for its existence.

metlos commented 1 month ago

There may be breakage in other parts of the testsuite in origin, if we update the samples from DeploymentConfig to Deployment. I'm not sure if mysql is used in any of the tests in https://github.com/openshift/origin/tree/master/test/extended/image_ecosystem or https://github.com/openshift/origin/tree/master/test/extended/builds or anywhere else.

metlos commented 1 month ago

/test ci/prow/e2e-aws-ovn-image-ecosystem

openshift-ci[bot] commented 1 month ago

@metlos: The specified target(s) for /test were not found. The following commands are available to trigger required jobs:

The following commands are available to trigger optional jobs:

Use /test all to run the following jobs that were automatically triggered:

In response to [this](https://github.com/openshift/cluster-samples-operator/pull/571#issuecomment-2375230815): >/test ci/prow/e2e-aws-ovn-image-ecosystem Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
metlos commented 1 month ago

/test e2e-aws-ovn-image-ecosystem

Prashanth684 commented 1 month ago

There may be breakage in other parts of the testsuite in origin, if we update the samples from DeploymentConfig to Deployment. I'm not sure if mysql is used in any of the tests in https://github.com/openshift/origin/tree/master/test/extended/image_ecosystem or https://github.com/openshift/origin/tree/master/test/extended/builds or anywhere else.

For okd, the extended test suite is not run and yes, we do need to be aware that if we ever do - this is a potential problem to solve. But I believe as far the conformance e2es are concerned, we are good

metlos commented 1 month ago

/approve

openshift-ci[bot] commented 1 month ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ausil, metlos, Prashanth684

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: - ~~[OWNERS](https://github.com/openshift/cluster-samples-operator/blob/master/OWNERS)~~ [metlos] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
openshift-ci-robot commented 1 month ago

/retest-required

Remaining retests: 0 against base HEAD 21a101bdbd3119406f2860781f5f8a7d02c721a3 and 2 for PR HEAD b34dd1e7292379b5bcc1788249494bca0e798059 in total

openshift-ci[bot] commented 1 month ago

@ausil: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
openshift-bot commented 1 month ago

[ART PR BUILD NOTIFIER]

Distgit: ose-cluster-samples-operator This PR has been included in build ose-cluster-samples-operator-container-v4.18.0-202409260508.p0.g1810acc.assembly.stream.el9. All builds following this will include this PR.