Bug 1779933: set version to start for s390/ppc until we have actual samples #205

gabemontero · 2019-12-05T14:37:52Z

openshift-ci-robot · 2019-12-05T14:37:57Z

@gabemontero: This pull request references Bugzilla bug 1779933, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1779933: set version to start for s390/ppc until we have actual samples

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.

openshift-ci-robot · 2019-12-05T14:37:57Z

@gabemontero: This pull request references Bugzilla bug 1779933, which is valid. The bug has been moved to the POST state.

In response to this:

Bug 1779933: set version to start for s390/ppc until we have actual samples

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.

bparees · 2019-12-05T14:42:18Z

pkg/stub/handler.go

@@ -588,6 +588,10 @@ func (h *Handler) Handle(event util.Event) error {
 		// Every time we see a change to the Config object, update the ClusterOperator status
 		// based on the current conditions of the Config.
 		cfg = h.refetchCfgMinimizeConflicts(cfg)
+		//TODO remove this setting of version once we start getting samples for z or ppc
+		if len(cfg.Spec.Architectures) > 0 && cfg.Spec.Architectures[0] != v1.AMDArchitecture && cfg.Spec.Architectures[0] != v1.X86Architecture {


i'm a little confused as to why this is specifically checking array index 0?

do we support multiple architectures being specified?

if we do, what happens if x86 is listed first and Z is listed second?

i'm a little confused as to why this is specifically checking array index 0?

1. do we support multiple architectures being specified?

at the moment, no ... we just have a place holder given this is an array to support it in the future

2. if we do, what happens if x86 is listed first and Z is listed second?

we hard code the entry at start up, and the first time through should set all the clusteroperator related bits

adding validation type logic that handles a cluster admin replacing our boostrapping with incorrect choices is doable, but more complicated

where we compare what they set in the array with what the golang arch env vars reports, etc.

gabemontero · 2019-12-05T15:03:26Z

/cherrypick release-4.3

openshift-cherrypick-robot · 2019-12-05T15:03:27Z

@gabemontero: once the present PR merges, I will cherry-pick it on top of release-4.3 in a new PR and assign it to you.

In response to this:

/cherrypick release-4.3

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.

openshift-cherrypick-robot · 2019-12-05T15:03:27Z

@gabemontero: once the present PR merges, I will cherry-pick it on top of release-4.3 in a new PR and assign it to you.

In response to this:

/cherrypick release-4.3

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.

gabemontero · 2019-12-05T15:03:42Z

/cherrypick release-4.2

openshift-cherrypick-robot · 2019-12-05T15:03:42Z

@gabemontero: once the present PR merges, I will cherry-pick it on top of release-4.2 in a new PR and assign it to you.

In response to this:

/cherrypick release-4.2

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.

openshift-cherrypick-robot · 2019-12-05T15:03:43Z

@gabemontero: once the present PR merges, I will cherry-pick it on top of release-4.2 in a new PR and assign it to you.

In response to this:

/cherrypick release-4.2

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.

adambkaplan

Since we need to backport this change and don't have e2e CI for Z/power, is there an easy way for us to add unit tests to verify we're setting version correctly?

adambkaplan · 2019-12-05T15:09:11Z

pkg/stub/handler.go

@@ -588,6 +588,10 @@ func (h *Handler) Handle(event util.Event) error {
 		// Every time we see a change to the Config object, update the ClusterOperator status
 		// based on the current conditions of the Config.
 		cfg = h.refetchCfgMinimizeConflicts(cfg)
+		//TODO remove this setting of version once we start getting samples for z or ppc
+		if len(cfg.Spec.Architectures) > 0 && cfg.Spec.Architectures[0] != v1.AMDArchitecture && cfg.Spec.Architectures[0] != v1.X86Architecture {
+			cfg.Status.Version = h.version


Shouldn't we always set this, and vary whether or not it is reported based on the Available status?

no we shouldn't ... all that was sorted out in 4.0/4.1 initial dev

only report the version once you have achieved it

pkg/stub/handler.go

gabemontero · 2019-12-05T15:25:34Z

Since we need to backport this change and don't have e2e CI for Z/power, is there an easy way for us to add unit tests to verify we're setting version correctly?

Yes I believe so ... will start on that

gabemontero · 2019-12-05T15:39:25Z

unit test added @adambkaplan

adambkaplan

@gabemontero need to check that version is set if we're reporting Available=true; Progressing=false, Degraded=false

adambkaplan · 2019-12-05T16:56:18Z

pkg/stub/handler_test.go

}
+

It doesn't look like validate verifies that we've set version correctly. Needs to be updated.

the setting of the version happens after the setup() call ... it does not need to be in validate

we don't need it everwhere

nevermind ... I'll do it everywhere

unit test updated

gabemontero · 2019-12-05T17:17:44Z

@gabemontero need to check that version is set if we're reporting Available=true; Progressing=false, Degraded=false

we don't test that in the unit tests ... we already do in the e2e's

https://github.com/openshift/cluster-samples-operator/blob/master/test/e2e/cluster_samples_operator_test.go#L219-L242

too big of a change to do in the unit tests

gabemontero · 2019-12-05T17:27:47Z

/test unit

wking · 2019-12-05T17:45:18Z

pkg/stub/handler_test.go

@@ -98,6 +107,7 @@ func TestWithArchDist(t *testing.T) {
 		conditions,
 		statuses, t)

+


nit: why do we keep adding empty lines?

gabemontero · 2019-12-05T18:49:14Z

/test e2e-aws

adambkaplan

/lgtm

We have existing coverage to verify version is set via the current e2e suite. However we do not have CI infrastructure for non-X86 architectures at present.

Multi-arch team will be manually verifying our patch to 4.2.z (which cannot be cherry-picked).

openshift-ci-robot · 2019-12-05T18:55:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, gabemontero

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~~ [adambkaplan,gabemontero]

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 · 2019-12-05T20:18:03Z

@gabemontero: All pull requests linked via external trackers have merged. Bugzilla bug 1779933 has been moved to the MODIFIED state.

In response to this:

Bug 1779933: set version to start for s390/ppc until we have actual samples

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.

openshift-ci-robot · 2019-12-05T20:18:03Z

@gabemontero: All pull requests linked via external trackers have merged. Bugzilla bug 1779933 has been moved to the MODIFIED state.

In response to this:

Bug 1779933: set version to start for s390/ppc until we have actual samples

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.

openshift-cherrypick-robot · 2019-12-05T20:18:13Z

@gabemontero: failed to push cherry-picked changes in GitHub: pushing failed, output: "To https://github.com/openshift-cherrypick-robot/cluster-samples-operator\n ! [remote rejected] cherry-pick-205-to-release-4.3 -> cherry-pick-205-to-release-4.3 (cannot lock ref 'refs/heads/cherry-pick-205-to-release-4.3': reference already exists)\nerror: failed to push some refs to 'https://openshift-cherrypick-robot:CENSORED@github.com/openshift-cherrypick-robot/cluster-samples-operator'\n", error: exit status 1

In response to this:

/cherrypick release-4.3

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.

openshift-cherrypick-robot · 2019-12-05T20:18:13Z

@gabemontero: new pull request created: #208

In response to this:

/cherrypick release-4.3

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.

openshift-cherrypick-robot · 2019-12-05T20:18:18Z

@gabemontero: #205 failed to apply on top of branch "release-4.2":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	pkg/operatorstatus/operatorstatus.go
M	pkg/stub/handler.go
M	pkg/stub/handler_test.go
A	pkg/util/util.go
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): pkg/util/util.go deleted in HEAD and modified in set version to start for s390/ppc until we have actual samples. Version set version to start for s390/ppc until we have actual samples of pkg/util/util.go left in tree.
Auto-merging pkg/stub/handler_test.go
Auto-merging pkg/stub/handler.go
Auto-merging pkg/operatorstatus/operatorstatus.go
Patch failed at 0001 set version to start for s390/ppc until we have actual samples

In response to this:

/cherrypick release-4.2

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.

openshift-cherrypick-robot · 2019-12-05T20:18:19Z

@gabemontero: #205 failed to apply on top of branch "release-4.2":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	pkg/operatorstatus/operatorstatus.go
M	pkg/stub/handler.go
M	pkg/stub/handler_test.go
A	pkg/util/util.go
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): pkg/util/util.go deleted in HEAD and modified in set version to start for s390/ppc until we have actual samples. Version set version to start for s390/ppc until we have actual samples of pkg/util/util.go left in tree.
Auto-merging pkg/stub/handler_test.go
Auto-merging pkg/stub/handler.go
Auto-merging pkg/operatorstatus/operatorstatus.go
Patch failed at 0001 set version to start for s390/ppc until we have actual samples

In response to this:

/cherrypick release-4.2

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.

openshift-ci-robot assigned adambkaplan and bparees Dec 5, 2019

openshift-ci-robot requested review from adambkaplan, dmage and coreydaley December 5, 2019 14:38

bparees reviewed Dec 5, 2019

View reviewed changes

gabemontero mentioned this pull request Dec 5, 2019

Revert Z/ppc support #204

Closed

adambkaplan suggested changes Dec 5, 2019

View reviewed changes

gabemontero force-pushed the s390x-version branch from 7d93162 to 55bf486 Compare December 5, 2019 15:24

openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 5, 2019

gabemontero force-pushed the s390x-version branch from 55bf486 to 6e00545 Compare December 5, 2019 15:39

openshift-ci-robot removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 5, 2019

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 5, 2019

gabemontero mentioned this pull request Dec 5, 2019

Bug 1779935: set version to start for s390/ppc until we have actual samples #206

Merged

adambkaplan suggested changes Dec 5, 2019

View reviewed changes

set version to start for s390/ppc until we have actual samples

4a582ed

gabemontero force-pushed the s390x-version branch from 6e00545 to 4a582ed Compare December 5, 2019 17:23

wking reviewed Dec 5, 2019

View reviewed changes

adambkaplan approved these changes Dec 5, 2019

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 5, 2019

openshift-merge-robot merged commit 32efce3 into openshift:master Dec 5, 2019

openshift-cherrypick-robot mentioned this pull request Dec 5, 2019

[release-4.3] Bug 1779934: set version to start for s390/ppc until we have actual samples #208

Merged

gabemontero deleted the s390x-version branch December 5, 2019 20:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1779933: set version to start for s390/ppc until we have actual samples #205

Bug 1779933: set version to start for s390/ppc until we have actual samples #205

gabemontero commented Dec 5, 2019

openshift-ci-robot commented Dec 5, 2019

openshift-ci-robot commented Dec 5, 2019

bparees Dec 5, 2019

gabemontero Dec 5, 2019 •

edited

gabemontero Dec 5, 2019 •

edited

gabemontero commented Dec 5, 2019

openshift-cherrypick-robot commented Dec 5, 2019

openshift-cherrypick-robot commented Dec 5, 2019

gabemontero commented Dec 5, 2019

openshift-cherrypick-robot commented Dec 5, 2019

openshift-cherrypick-robot commented Dec 5, 2019

adambkaplan left a comment

adambkaplan Dec 5, 2019

gabemontero Dec 5, 2019

gabemontero commented Dec 5, 2019

gabemontero commented Dec 5, 2019

adambkaplan left a comment

adambkaplan Dec 5, 2019

gabemontero Dec 5, 2019

gabemontero Dec 5, 2019

gabemontero Dec 5, 2019

gabemontero Dec 5, 2019

gabemontero commented Dec 5, 2019

gabemontero commented Dec 5, 2019

wking Dec 5, 2019

gabemontero commented Dec 5, 2019

adambkaplan left a comment

openshift-ci-robot commented Dec 5, 2019

openshift-ci-robot commented Dec 5, 2019

openshift-ci-robot commented Dec 5, 2019

openshift-cherrypick-robot commented Dec 5, 2019

openshift-cherrypick-robot commented Dec 5, 2019

openshift-cherrypick-robot commented Dec 5, 2019

openshift-cherrypick-robot commented Dec 5, 2019

		@@ -98,6 +107,7 @@ func TestWithArchDist(t *testing.T) {
		conditions,
		statuses, t)

Bug 1779933: set version to start for s390/ppc until we have actual samples #205

Bug 1779933: set version to start for s390/ppc until we have actual samples #205

Conversation

gabemontero commented Dec 5, 2019

openshift-ci-robot commented Dec 5, 2019

openshift-ci-robot commented Dec 5, 2019

Choose a reason for hiding this comment

gabemontero Dec 5, 2019 • edited

Choose a reason for hiding this comment

gabemontero Dec 5, 2019 • edited

Choose a reason for hiding this comment

gabemontero commented Dec 5, 2019

openshift-cherrypick-robot commented Dec 5, 2019

openshift-cherrypick-robot commented Dec 5, 2019

gabemontero commented Dec 5, 2019

openshift-cherrypick-robot commented Dec 5, 2019

openshift-cherrypick-robot commented Dec 5, 2019

adambkaplan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabemontero commented Dec 5, 2019

gabemontero commented Dec 5, 2019

adambkaplan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabemontero commented Dec 5, 2019

gabemontero commented Dec 5, 2019

Choose a reason for hiding this comment

gabemontero commented Dec 5, 2019

adambkaplan left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Dec 5, 2019

openshift-ci-robot commented Dec 5, 2019

openshift-ci-robot commented Dec 5, 2019

openshift-cherrypick-robot commented Dec 5, 2019

openshift-cherrypick-robot commented Dec 5, 2019

openshift-cherrypick-robot commented Dec 5, 2019

openshift-cherrypick-robot commented Dec 5, 2019

gabemontero Dec 5, 2019 •

edited

gabemontero Dec 5, 2019 •

edited