Skip to content

Conversation

@pacevedom
Copy link
Contributor

@pacevedom pacevedom commented Aug 5, 2022

Add a function to determine if the cluster is a MicroShift instance.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 5, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 5, 2022

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@pacevedom pacevedom marked this pull request as ready for review August 16, 2022 16:08
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 16, 2022
@openshift-ci openshift-ci bot requested review from gabemontero and knobunc August 16, 2022 16:09
@pacevedom
Copy link
Contributor Author

/retest-required

1 similar comment
@pacevedom
Copy link
Contributor Author

/retest-required

@pacevedom
Copy link
Contributor Author

/retest-required

2 similar comments
@pacevedom
Copy link
Contributor Author

/retest-required

@pacevedom
Copy link
Contributor Author

/retest-required

@dhellmann
Copy link
Contributor

The implementation looks good. Are those CI job failures related?

@pacevedom
Copy link
Contributor Author

The implementation looks good. Are those CI job failures related?

Not really, this function is not used yet. Anyway I tmeporarily removed the call from GetControlPlaneTopology to inspect if we might enable bad code paths in other tests.

@pacevedom
Copy link
Contributor Author

/retest-required

Copy link
Contributor

@dhellmann dhellmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit that can be addressed later.

/approve

@ggiguash
Copy link

ggiguash commented Aug 31, 2022

/retitle USHIFT-285: Add MicroShift version getter to framework

@openshift-ci openshift-ci bot changed the title Add microshift version getter to framework USHIFT-285: Add MicroShift version getter to framework Aug 31, 2022
Copy link
Contributor

@dhellmann dhellmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 31, 2022
@pacevedom
Copy link
Contributor Author

/assign @knobunc

@pacevedom
Copy link
Contributor Author

/retest-required

3 similar comments
@pacevedom
Copy link
Contributor Author

/retest-required

@pacevedom
Copy link
Contributor Author

/retest-required

@ggiguash
Copy link

ggiguash commented Sep 4, 2022

/retest-required

@pacevedom
Copy link
Contributor Author

/assign @ingvagabund

defer microShiftMutex.Unlock()

if microShiftVersion == "" {
cm, err := ocClient.AdminKubeClient().CoreV1().ConfigMaps("kube-public").Get(context.Background(), "microshift-version", metav1.GetOptions{})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any microshift-version CM under kube-public NS in ocp 4.12 vanilla cluster. Meaning get request for the CM will get requested every time a test invoking IsMicroShiftDeployment gets running. What about introducing another microShiftVersionCMNotFound variable and setting it to false when err != nil && apierrors.IsNotFound(err)? Then if microShiftVersionCMNotFound { return "" } before if microShiftVersion == "" {? Or similar mechanism against re-getting microshift-version` CM.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 21, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dhellmann, pacevedom
Once this PR has been reviewed and has the lgtm label, please ask for approval from knobunc by writing /assign @knobunc in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@deads2k
Copy link
Contributor

deads2k commented Sep 21, 2022

/hold

let's see where this is needed instead of using existing external signals before merging.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 21, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 22, 2022

New changes are detected. LGTM label has been removed.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 22, 2022

@pacevedom: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-single-node-upgrade 0225280 link false /test e2e-aws-single-node-upgrade
ci/prow/e2e-aws-single-node 0225280 link false /test e2e-aws-single-node
ci/prow/e2e-agnostic-cmd 0225280 link false /test e2e-agnostic-cmd
ci/prow/e2e-aws-ovn-single-node-upgrade 648b238 link false /test e2e-aws-ovn-single-node-upgrade
ci/prow/e2e-aws-ovn-serial 648b238 link true /test e2e-aws-ovn-serial
ci/prow/e2e-gcp-ovn-upgrade 648b238 link true /test e2e-gcp-ovn-upgrade
ci/prow/e2e-aws-ovn-single-node 648b238 link false /test e2e-aws-ovn-single-node
ci/prow/e2e-aws-ovn-single-node-serial 648b238 link false /test e2e-aws-ovn-single-node-serial

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@pacevedom
Copy link
Contributor Author

/hold

let's see where this is needed instead of using existing external signals before merging.

So it's these tests now:

"[sig-auth][Feature:PodSecurity] restricted-v2 SCC should mutate empty securityContext to match restricted PSa profile [Suite:openshift/conformance/parallel]"
"[sig-auth][Feature:OpenShiftAuthorization] The default cluster RBAC policy should have correct RBAC rules [Suite:openshift/conformance/parallel]"
"[sig-coreos] [Conformance] CoreOS bootimages TestBootimagesPresent [Suite:openshift/conformance/parallel/minimal]"
"[sig-devex][Feature:OpenShiftControllerManager] TestAutomaticCreationOfPullSecrets [apigroup:config.openshift.io] [Suite:openshift/conformance/parallel]"
"[sig-devex][Feature:OpenShiftControllerManager] TestDockercfgTokenDeletedController [Suite:openshift/conformance/parallel]"
"[sig-imageregistry][Feature:ImageInfo] Image info should display information about images [Skipped:Disconnected] [Suite:openshift/conformance/parallel]"
"[sig-network][Feature:Router][apigroup:route.openshift.io] The HAProxy router converges when multiple routers are writing conflicting status [Suite:openshift/conformance/parallel]"
"[sig-network][Feature:Router][apigroup:route.openshift.io] The HAProxy router converges when multiple routers are writing status [Suite:openshift/conformance/parallel]"

More to come as we create/modify specific tests for microshift, I would say.

@pacevedom
Copy link
Contributor Author

Not needed anymore, as we look for dependencies in each test.

@pacevedom pacevedom closed this Sep 26, 2022
@pacevedom pacevedom deleted the microshift branch September 26, 2022 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants