Skip to content

Bug 1868068: Evaluate daemonset init containers during reconciliation #669

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
Merged
merged 1 commit into from
Aug 25, 2020
Merged

Bug 1868068: Evaluate daemonset init containers during reconciliation #669

merged 1 commit into from
Aug 25, 2020

Conversation

@jcantrill
Copy link
Contributor

@jcantrill jcantrill commented Aug 21, 2020

This PR:

  • Evaluates daemonset init containers when reconciling the collector
  • Refactors daemonset comparison to no longer mutate incoming daemonsets
  • Renames the method to retrieve the TrustedCAHash to be a getter instead of getter/mutator

ref: https://bugzilla.redhat.com/show_bug.cgi?id=1868068

@openshift-ci-robot openshift-ci-robot added the bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. label Aug 21, 2020
@openshift-ci-robot
Copy link

openshift-ci-robot commented Aug 21, 2020

@jcantrill: This pull request references Bugzilla bug 1868068, 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.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.6.0) matches configured target release for branch (4.6.0)
  • bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1868068: Evaluate daemonset init containers during reconciliation

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 openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Aug 21, 2020
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 21, 2020
alanconway
alanconway approved these changes Aug 21, 2020
View reviewed changes
Copy link
Contributor

@alanconway alanconway left a comment

Choose a reason for hiding this comment

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

/lgtm
Style note (no need to change) - when you have a series of if statements each of which returns, I find it neater to use a case statement. Not important enough to change the PR

@openshift-ci-robot
Copy link

openshift-ci-robot commented Aug 21, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alanconway, jcantrill

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 [alanconway,jcantrill]

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

ewolinetz commented Aug 21, 2020 •
edited
Loading

/hold

Refactors daemonset comparison to no longer mutate incoming daemonsets

why not? or maybe you can clarify on this more...

Renames the method to retrieve the TrustedCAHash to be a getter instead of getter/mutator

nit -- but it wasn't a mutator before, don't understand how this resolves the bz

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 21, 2020
@jcantrill
Copy link
Contributor Author

jcantrill commented Aug 21, 2020

/lgtm
Style note (no need to change) - when you have a series of if statements each of which returns, I find it neater to use a case statement. Not important enough to change the PR

Can you please provide an example as I'm curious how one might do that with a switch

@jcantrill
Copy link
Contributor Author

jcantrill commented Aug 21, 2020

Refactors daemonset comparison to no longer mutate incoming daemonsets

why not? or maybe you can clarify on this more...

Functions and methods should me named and provide behavior that:

  • distinctly Identifies what it is doing
  • provide the least element of surprise
    isFluentdDaemonsetDifferent violated both of these as well as complicated testing. The diff code, in general, has an abundance of places where it says one thing and does much more and IMO is just bad practice. Mutation in the statement is specifically targeted to what was the function supposed to do and not do.

Renames the method to retrieve the TrustedCAHash to be a getter instead of getter/mutator
nit -- but it wasn't a mutator before,

It actually was a mutator in the fact it sent in the DS and then fetched a CM to calc a hash to then add it to the DS annotations.

don't understand how this resolves the bz
It was only needed to solve the bug in context of the other bits that were changed. I admit this is an aggressive "fix" leaning more on refactoring but IMO it's worth the risk given the state of the code needs attention

@jcantrill
Copy link
Contributor Author

jcantrill commented Aug 21, 2020

/refresh

@alanconway
Copy link
Contributor

alanconway commented Aug 24, 2020

/lgtm
Style note (no need to change) - when you have a series of if statements each of which returns, I find it neater to use a case statement. Not important enough to change the PR

Can you please provide an example as I'm curious how one might do that with a switch

Sorry, doesn't apply here. You can do a switch for simple boolean expressions, but you can't do an inline assignment in a case statement (I keep forgetting that) so it doesn't work for: err=x(); err != nil

@jcantrill
Copy link
Contributor Author

jcantrill commented Aug 24, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2020
@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 24, 2020

/retest

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

@jcantrill
Copy link
Contributor Author

jcantrill commented Aug 25, 2020

/refresh

@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 25, 2020

/retest

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

7 similar comments
@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 25, 2020

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 25, 2020

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 25, 2020

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 25, 2020

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 25, 2020

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 25, 2020

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 25, 2020

/retest

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

@jcantrill
Copy link
Contributor Author

jcantrill commented Aug 25, 2020

/retest

@openshift-merge-robot openshift-merge-robot merged commit c6dea22 into openshift:master Aug 25, 2020
@openshift-ci-robot
Copy link

openshift-ci-robot commented Aug 25, 2020

@jcantrill: All pull requests linked via external trackers have merged:

Bugzilla bug 1868068 has been moved to the MODIFIED state.

In response to this:

Bug 1868068: Evaluate daemonset init containers during reconciliation

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.

pmoogi-redhat pushed a commit to pmoogi-redhat/cluster-logging-operator that referenced this pull request Apr 26, 2022
@openshift-merge-robot
Merge pull request openshift#669 from jcantrill/1868068
c4f61aa 
Bug 1868068: Evaluate daemonset init containers during reconciliation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ewolinetz ewolinetz Awaiting requested review from ewolinetz

@vimalk78 vimalk78 Awaiting requested review from vimalk78

1 more reviewer

@alanconway alanconway alanconway approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@alanconway alanconway

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jcantrill @openshift-ci-robot @ewolinetz @alanconway @openshift-bot @openshift-merge-robot