-
Notifications
You must be signed in to change notification settings - Fork 16.8k
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: davidkarlsen 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 |
/do-not-merge |
/assign @mattfarina - care to comment? |
Signed-off-by: David J. M. Karlsen <david@davidkarlsen.com>
f2f7b4c
to
ba8d4f1
Compare
I've started #7692 as well, maybe we can do in common? I have to update it to state a few things. |
REVIEW_GUIDELINES.md
Outdated
|
||
**NOTE:** | ||
*The guidelines for standard labels have changed, but due to labels being immutable, charts with the | ||
legacy labels should continue to use the old label-style, while new charts should start using the updated labels.* |
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.
In fact, for charts still using extensions/v1beta1 or apps/v1beta1, it is not immutable so there is a opportunity for them to upgrade.
Signed-off-by: David J. M. Karlsen <david@davidkarlsen.com>
/hold use hold to apply the do not merge label |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions. |
This issue is being automatically closed due to inactivity. |
Should we reopen this and finish it? |
This LGTM, @davidkarlsen have you had a chance to test the mutating label issue? |
REVIEW_GUIDELINES.md
Outdated
|
||
**NOTE:** | ||
*The guidelines for standard labels have changed, but due to labels being immutable when using apps/v1, charts with the | ||
legacy labels should continue to use the old label-style, while new charts should start using the updated labels.* |
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.
Do we want all charts to use new labels in the long term? If so it should be stated that it can be done along with another needed breaking change.
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 agree with Matt's comment #9332 (review). The consensus in meetings so far has been that we shouldn't unnecessarily make breaking changes to charts, and updating to the new labels isn't a good enough reason. But if there are going to be significant enough breaking changes to a chart anyway, it could be a good time to update to the new labels in the same PR.
@davidkarlsen I just suggested a clarification in a PR on your fork against this PR's branch to : davidkarlsen#2 in case that helps 😄
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.
For quick ref, this is my suggestion in that PR:
*The guidelines for standard labels have changed, but due to labels being immutable when using apps/v1, charts with the The guidelines for standard labels have changed. However, since labels are immutable when using apps/v1, and the legacy labels still function, updating a chart from legacy to new labels would be an unnecessary breaking change. Therefore, charts with legacy labels should continue to use them until another breaking change pull request presents an opportunity to also update to the new label style in one MAJOR version bump.
WDYT?
… the new label style Signed-off-by: Scott Rigby <scott@r6by.com>
PR wording change suggestion
@davidkarlsen: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions. |
This issue is being automatically closed due to inactivity. |
fixes #7550
This is not done - but a start to discuss and iterate on
/do-not-merge
NOTE!
The updated labels actually have mutable values - won't this be a problem when used with deployment api v1? As discussed in the meeting?