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.

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

static pod operator should not update versions until the image is new #865

Merged
merged 1 commit into from Aug 17, 2020

Conversation

)
c.versionRecorder.SetVersion(
"operator",
status.VersionForOperatorFromEnv(),
Copy link
Member

Choose a reason for hiding this comment

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

Is this setting the operator version for the operator as a whole, or just for this one static pod controller (with the operator's operator version leveling after all its sub-controllers have leveled)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this setting the operator version for the operator as a whole, or just for this one static pod controller (with the operator's operator version leveling after all its sub-controllers have leveled)?

it's voting. anything needing a different version sets it separately. But static pod operators don't get choices, it changes when their static pods are up to date.

Copy link
Member

Choose a reason for hiding this comment

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

...it's voting. anything needing a different version sets it separately...

Is there aggregation logic that tallies votes somewhere, similar to what UnionClusterCondition does for conditions? Grepping for "operator" is not turning up an aggregator for me...

Copy link
Member

Choose a reason for hiding this comment

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

For what it's worth, I'd expect each controller to set an operandName, as you have a bit further up here. And then operator-level logic that looks at all the controllers and bumps operator when they all match their target operand version.


default: // we have one image
// if have a consistent image and if that image the same as the current operand image, then we can update the version to reflect our new version
if images.List()[0] == status.ImageForOperandFromEnv() {
Copy link
Member

Choose a reason for hiding this comment

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

nit:

case images.List()[0] == status.ImageForOperandFromEnv():
  ...
default:
  // we have one image, but it is NOT the current operand image so we don't update the version
}

would unroll one level of nesting, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the intent it clearer as is.

Copy link
Contributor

@sanchezl sanchezl left a comment

Choose a reason for hiding this comment

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

/lgtm


default: // we have one image
// if have a consistent image and if that image the same as the current operand image, then we can update the version to reflect our new version
if images.List()[0] == status.ImageForOperandFromEnv() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the intent it clearer as is.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 17, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, sanchezl

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 5e77ffd into openshift:master Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants