-
Notifications
You must be signed in to change notification settings - Fork 129
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
Correctly sync deployment image #123
Conversation
ed78f9e
to
8a5128b
Compare
bab163d
to
8148985
Compare
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR looks good 👍 just some nits & a question :)
Also I really like how the deployment sync got simplified.
@jhadvig I think for the unit tests we specifically don't want to use |
ohh.. if the |
Still WIP? Tests pass, def simpler. |
Simplify `SyncDeployment` and use deployment annotations to force a rollout when the console image changes.
@jhadvig @benjaminapetersen thanks, this is ready now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good. Let's wait for prow to for the status so we can merge.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, spadgett 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 |
/retest not much happening with statuses. |
lgtm if ci gets happy... |
/retest |
2 similar comments
/retest |
/retest |
/retest |
/retest |
1 similar comment
/retest |
return nil, false, updateErr | ||
} | ||
return updatedDeployment, depChanged, nil | ||
expectedGeneration := getDeploymentGeneration(co) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we still have work to do here, with generation, per our conversation. Should we comment to track, or assume a fast follow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's open a JIRA issue
/retest |
3 similar comments
/retest |
/retest |
/retest |
Simplify
SyncDeployment
and use deployment annotations to force a rollout when the console image changes.@benjaminapetersen @enj