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.

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

Chart: Improve #10673. #10721

Merged
merged 1 commit into from
Dec 20, 2023
Merged

Chart: Improve #10673. #10721

merged 1 commit into from
Dec 20, 2023

Conversation

Gacko
Copy link
Member

@Gacko Gacko commented Dec 3, 2023 •
edited
Loading

What this PR does / why we need it:

#10673 (comment)

Closes #10760.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • CVE Report (Scanner found CVE and adding report)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation only

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have added unit and/or e2e tests to cover my changes.
  • All new and existing tests passed.

@netlify Netlify
Copy link

netlify bot commented Dec 3, 2023 •
edited
Loading

✅ Deploy Preview for kubernetes-ingress-nginx canceled.

Name Link
🔨 Latest commit 2f7f4d7
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/65815bfa0f0a5f000837a304

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 3, 2023
@k8s-ci-robot k8s-ci-robot added area/helm Issues or PRs related to helm charts approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 3, 2023
@Gacko
Copy link
Member Author

Gacko commented Dec 3, 2023

/assign @cpanato

@Gacko Gacko mentioned this pull request Dec 3, 2023
Closed
@cpanato
Copy link
Member

cpanato commented Dec 3, 2023

this will not fail when we merge?

@cpanato
Copy link
Member

cpanato commented Dec 3, 2023

/assign @strongjz

@strongjz
Copy link
Member

strongjz commented Dec 3, 2023

Yea, i don't think the helm releaser will run. A roll forward is the better option.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 3, 2023
@Gacko
Copy link
Member Author

Gacko commented Dec 3, 2023 •
edited
Loading

This is what chart release v4.8.4 contains:

helm-chart-4.8.3...helm-chart-4.8.4

Whatever we do from here, we can not release v4.8.5 without the above changes on top of main. So if you'd like to do a v4.8.5, it would need to be forged on tag helm-chart-v4.8.3.

@cpanato
Copy link
Member

cpanato commented Dec 3, 2023 •
edited
Loading

yep, thinking a bit, we will need to revert and bump the chart to 4.8.5, otherwise the ci in the postsubmit will fail, because the 4.8.3 already exists

in other helm chart repos we do like this, revert and bump to the next version, and mark the bad release as to to use in the changelog, and the new release is just a no-op

i've reopen the pr that reverts and bump the chart.

@Gacko
Copy link
Member Author

Gacko commented Dec 3, 2023

We don't need to revert the PR itself, it's basically fine, maybe only add the changes I did to it. But apart from that we should not release a v4.8.5 either, as it would still be a patch release containing changes requiring rather a minor release (new features, not only bugfixes).

@cpanato
Copy link
Member

cpanato commented Dec 3, 2023

discussing over slack: https://kubernetes.slack.com/archives/C021E147ZA4/p1701621660689399

@cpanato
Copy link
Member

cpanato commented Dec 3, 2023

ok, we decided to make a minor release and move forward, we will create a changelog for all changes

@Gacko Gacko changed the title Chart: Revert version bump from #10673. Chart: Align & improve changes from #10673. Dec 3, 2023
@Gacko
Copy link
Member Author

Gacko commented Dec 3, 2023

Current state: I reverted the version changing things. For now this PR is only about adding the forgotten component label, aligning & improving templating to other resources.

I'll have a look into the missing changelog file tomorrow. Hopefully.

@Gacko
Copy link
Member Author

Gacko commented Dec 5, 2023

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Dec 5, 2023
@Gacko
Copy link
Member Author

Gacko commented Dec 6, 2023

/kind cleanup

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Dec 6, 2023
@Gacko Gacko force-pushed the k4as9 branch 3 times, most recently from bbaa251 to 80486f2 Compare December 7, 2023 18:17
@Gacko Gacko changed the title Chart: Align & improve changes from #10673. Chart: Improve changes from #10673 and add changelog. Dec 7, 2023
@Gacko Gacko changed the title Chart: Improve changes from #10673 and add changelog. Chart: Improve #10673 and add changelog. Dec 7, 2023
@Gacko Gacko force-pushed the k4as9 branch 8 times, most recently from 03e583e to d1225be Compare December 14, 2023 15:29
@Gacko Gacko force-pushed the k4as9 branch 2 times, most recently from 367e3a0 to 5be6e5c Compare December 17, 2023 08:37
@Gacko Gacko changed the title Chart: Improve #10673 and add changelog. Chart: Improve #10673. Dec 17, 2023
@strongjz
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 20, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Gacko, strongjz

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

@strongjz
Copy link
Member

/cherry-pick release-1.9

@k8s-infra-cherrypick-robot
Copy link
Contributor

@strongjz: once the present PR merges, I will cherry-pick it on top of release-1.9 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.9

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.

@strongjz strongjz merged commit 97a0483 into kubernetes:main Dec 20, 2023
41 checks passed
@k8s-infra-cherrypick-robot
Copy link
Contributor

@strongjz: #10721 failed to apply on top of branch "release-1.9":

Applying: Chart: Improve #10673.
Using index info to reconstruct a base tree...
A	charts/ingress-nginx/templates/default-backend-extra-configmaps.yaml
A	charts/ingress-nginx/tests/controller-service_test.yaml
A	charts/ingress-nginx/tests/default-backend-extra-configmaps_test.yaml
M	charts/ingress-nginx/values.yaml
Falling back to patching base and 3-way merge...
Auto-merging charts/ingress-nginx/values.yaml
CONFLICT (content): Merge conflict in charts/ingress-nginx/values.yaml
CONFLICT (modify/delete): charts/ingress-nginx/tests/default-backend-extra-configmaps_test.yaml deleted in HEAD and modified in Chart: Improve #10673.. Version Chart: Improve #10673. of charts/ingress-nginx/tests/default-backend-extra-configmaps_test.yaml left in tree.
CONFLICT (modify/delete): charts/ingress-nginx/tests/controller-service_test.yaml deleted in HEAD and modified in Chart: Improve #10673.. Version Chart: Improve #10673. of charts/ingress-nginx/tests/controller-service_test.yaml left in tree.
CONFLICT (modify/delete): charts/ingress-nginx/templates/default-backend-extra-configmaps.yaml deleted in HEAD and modified in Chart: Improve #10673.. Version Chart: Improve #10673. of charts/ingress-nginx/templates/default-backend-extra-configmaps.yaml left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Chart: Improve #10673.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.9

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.

@Gacko Gacko deleted the k4as9 branch December 20, 2023 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpanato cpanato Awaiting requested review from cpanato

@strongjz strongjz Awaiting requested review from strongjz

Assignees

@strongjz strongjz

@cpanato cpanato

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/helm Issues or PRs related to helm charts cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Chart/Tests: Follow-up for #10731.
5 participants
@Gacko @cpanato @strongjz @k8s-ci-robot @k8s-infra-cherrypick-robot