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

Fix hook install order #12495

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

lynshi
Copy link

@lynshi lynshi commented Oct 14, 2023

What this PR does / why we need it: closes #12414

According to Hooks and the Release Lifecycle, hooks are sorted "by weight (assigning a weight of 0 by default), by resource kind and finally by name in ascending order". In practice, the current behavior is that hooks are sorted by (Weight, Name).

The code currently uses sort.Stable under the assumption that the hooks are already sorted by Kind. However, stability only applies to elements with the same key, so since Less on the hookByWeight comparator orders based on the name of the hook too, the existing ordering by Kind is never respected.

This PR fixes the comparator so that hooks are sorted by (Weight, Kind, Name) as expected and adds tests for this behavior.

Special notes for your reviewer:

If applicable:

  • this PR contains documentation
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

Signed-off-by: Lyndon Shi <shilyndon@outlook.com>
Signed-off-by: Lyndon Shi <shilyndon@outlook.com>
Signed-off-by: Lyndon Shi <shilyndon@outlook.com>
Signed-off-by: Lyndon Shi <shilyndon@outlook.com>
Signed-off-by: Lyndon Shi <shilyndon@outlook.com>
Signed-off-by: Lyndon Shi <shilyndon@outlook.com>
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 14, 2023
Signed-off-by: Lyndon Shi <shilyndon@outlook.com>
Signed-off-by: Lyndon Shi <shilyndon@outlook.com>
@lynshi
Copy link
Author

lynshi commented Nov 14, 2023

Only one of this and helm/helm-www#1520 should be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ServiceAccount is not created before Jobs/Pods
1 participant