Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Removed hardcoded portable=true #480

Merged
merged 1 commit into from
Apr 23, 2020
Merged

Removed hardcoded portable=true #480

merged 1 commit into from
Apr 23, 2020

Conversation

rohan47
Copy link
Contributor

@rohan47 rohan47 commented Apr 21, 2020

portable was hardcoded to true in case placement is none which resulted in portable=true even if in the storageCluster CR it was specified false

Signed-off-by: rohan47 rohgupta@redhat.com

@rohan47
Copy link
Contributor Author

rohan47 commented Apr 21, 2020

/cc @jarrpa

@openshift-ci-robot openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 21, 2020
@openshift-ci-robot
Copy link

Hi @rohan47. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

jarrpa
jarrpa requested changes Apr 21, 2020
View reviewed changes
Copy link
Member

@jarrpa jarrpa left a comment

Choose a reason for hiding this comment

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

The change is good. Please include a note in the commit message of why this is no longer needed.

travisn
travisn reviewed Apr 21, 2020
View reviewed changes
pkg/controller/storagecluster/reconcile.go
@@ -1000,7 +1000,6 @@ func newStorageClassDeviceSets(sc *ocsv1.StorageCluster) []rook.StorageClassDevi
(&in).DeepCopyInto(&placement)

if len(topologyKeyValues) >= replica {
portable = true
Copy link
Contributor

Choose a reason for hiding this comment

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

How about we also remove the variable on line 996 and then on line 1026 we use ds.Portable directly.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose we can, yeah. We can just introduce the variable again later if we have need.

@jarrpa
Copy link
Member

jarrpa commented Apr 21, 2020

An improved commit message, but there is a typo and could use a bit of rewording. Something like:

The OCP console used to require us to hardcode portable=true. Now it explicitly
sets portable=true so there is no longer need to hardcode it in the operator
code. Removing the hardcoded value from the operator code also makes it possible
to set the value of portable without setting a placement as a workaround.

jarrpa
jarrpa approved these changes Apr 21, 2020
View reviewed changes
Copy link
Member

@jarrpa jarrpa left a comment

Choose a reason for hiding this comment

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

Thanks!

/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 21, 2020
@jarrpa
Copy link
Member

jarrpa commented Apr 21, 2020

/retest

@umangachapagain
Copy link
Contributor

@rohan47 You need to run make gen-latest-csv. Seems like it is not accepted by CI.
Also, tests might need to be fixed.

@umangachapagain
Copy link
Contributor

/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 22, 2020
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

8 similar comments
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2020
@rohan47
Copy link
Contributor Author

rohan47 commented Apr 22, 2020 •
edited
Loading

@umangachapagain looks like even after make gen-latest-csv it's not accepted by CI

@obnoxxx
Copy link
Contributor

obnoxxx commented Apr 22, 2020

@rohan47 wrote:

looks like even after make gen-latest-csv it's not accepted by CI

Did you also commit the hack/latest-csv-checksum.md5 file (or so) and force-push to this pr's branch?

@rohan47
Copy link
Contributor Author

rohan47 commented Apr 22, 2020

@rohan47 wrote:

looks like even after make gen-latest-csv it's not accepted by CI

Did you also commit the hack/latest-csv-checksum.md5 file (or so) and force-push to this pr's branch?

Yes

@obnoxxx
Copy link
Contributor

obnoxxx commented Apr 22, 2020

Functional test failing here: https://github.com/openshift/ocs-operator/pull/480/files#diff-3ab4add16e230d7d70aa431f83ce9551R491

@obnoxxx
Copy link
Contributor

obnoxxx commented Apr 22, 2020

Did you also commit the hack/latest-csv-checksum.md5 file (or so) and force-push to this pr's branch?

Yes

Strange... I get a diff when i run make gen-latest-csv locally and the error to run ocs-operator-ci goes away once I commit it.

@rohan47
Copy link
Contributor Author

rohan47 commented Apr 22, 2020

Did you also commit the hack/latest-csv-checksum.md5 file (or so) and force-push to this pr's branch?

Yes

Strange... I get a diff when i run make gen-latest-csv locally and the error to run ocs-operator-ci goes away once I commit it.

Its the timestamp in deploy/olm-catalog/ocs-operator/4.5.0/ocs-operator.v4.5.0.clusterserviceversion.yaml and hack/latest-csv-checksum.md5 files. Not sure what is wrong here.

@rohan47 @obnoxxx
Removed hardcoded portable=true
a34c132 
The OCP console used to require us to hardcode portable=true.
Now it explicitly sets portable=true so there is no longer need to
hardcode it in the operator code. Removing the hardcoded value from
the operator code also makes it possible to set the value of portable
without setting a placement as a workaround.

Signed-off-by: rohan47 <rohgupta@redhat.com>
@obnoxxx
Copy link
Contributor

obnoxxx commented Apr 23, 2020

@rohan47 FYI I just updated the PR with my results of running make gen-latest-csv (squashed into your patch) to see if that fixes the issue. It passes make ocs-operator-ci locally for me.

I had a similar issue with another PR recently, and IIRC, the problem went away after I had run git clean -xdf... Let's see how this goes.

@openshift-ci-robot
Copy link

@rohan47: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/red-hat-storage-ocs-ci-e2e-aws a34c132 link /test red-hat-storage-ocs-ci-e2e-aws
ci/prow/ocs-operator-e2e-aws a34c132 link /test ocs-operator-e2e-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

jarrpa
jarrpa approved these changes Apr 23, 2020
View reviewed changes
Copy link
Member

@jarrpa jarrpa 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-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 23, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jarrpa, rohan47

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 7805794 into red-hat-storage:master Apr 23, 2020
@rohan47 rohan47 deleted the portable-unhardcode branch April 23, 2020 06:27
@openshift-ci-robot openshift-ci-robot mentioned this pull request Apr 23, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@travisn travisn travisn left review comments

@jarrpa jarrpa jarrpa approved these changes

@egafford egafford Awaiting requested review from egafford

@mhrivnak mhrivnak Awaiting requested review from mhrivnak

Assignees

@jarrpa jarrpa

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@rohan47 @openshift-ci-robot @jarrpa @umangachapagain @openshift-bot @obnoxxx @travisn @openshift-merge-robot