-
Notifications
You must be signed in to change notification settings - Fork 49
pkg/helm: add wrapper to get the Helm history we expect #1515
Conversation
fcbdaee
to
54860bf
Compare
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.
Hmm, content of the PR does not match the title.
Sorry, pushed to the wrong branch 😅 |
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.
Just some nits, otherwise looks OK
54860bf
to
d0209a0
Compare
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.
One nit, otherwise looks good 👍 Nice and clear addition, and well tested 👌
We expect to get the history in descending order by version and limited to 1 version to get the last version so we can ensure the user has the last version of the Helm chart. However, it turns out the Helm API doesn't honor the Max config parameter and returns the Helm history in a non-deterministic order. This adds a wrapper function that sorts what the Helm API returns in the way we expect and uses that. Fixes #1442
d0209a0
to
bea4a93
Compare
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.
LGTM 👍
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.
LGTM
@@ -309,9 +310,9 @@ func (c controlplaneUpdater) ensureComponent(component, namespace string) error | |||
} | |||
|
|||
histClient := action.NewHistory(actionConfig) | |||
histClient.Max = 1 | |||
histMax := 1 |
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.
What if the last one in the history was not a successful deploy? Do we want the last one in the history or the last successful deploy in the history?
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.
Good point 😐
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.
We could check for the last "deployed" instance.
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.
I'd say it's out of scope for this PR? Maybe create an issue and create a follow-up PR? This PR fixes existing assumed functionality, while suggestion from @surajssd changes the behavior from existing one, although I agree what we do right now is not fully correct.
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.
Alright sure, lets move forward. But yeah we will need a func that give the last successful deploy.
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.
We expect to get the history in descending order by version and limited
to 1 version to get the last version so we can ensure the user has the
last version of the Helm chart.
However, it turns out the Helm API doesn't honor the Max config parameter and
returns the Helm history in a non-deterministic order.
This adds a wrapper function that sorts what the Helm API returns in the
way we expect and uses that.
Fixes #1442