-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🌱 logging: add removal note, cleanup no-op code #7955
🌱 logging: add removal note, cleanup no-op code #7955
Conversation
Signed-off-by: Stefan Büringer [email protected]
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
@@ -41,6 +41,11 @@ maintainers of providers and consumers of our Go API. | |||
- `clusterctl upgrade apply` no longer requires a namespace when updating providers. It is now optional and in a future release it will be deprecated. The new syntax is `[namespace/]provider:version`. | |||
- `WatchDeploymentLogs` is changed to `WatchDeploymentLogsByName`, it works same as before. Another function `WatchDeploymentLogsByLabelSelector` is added to stream logs of deployment by label selector. | |||
- Cluster API controllers are now using an explicit security context by default. | |||
- It is recommended to drop usages of `logs.AddFlags(fs, logs.SkipLoggingConfigurationFlags())`. It was previously used to configure deprecated logging flags, but with the bump to component-base |
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.
@oscr I added this to the provider migration doc as it's relevant for provider authors as well.
But we also want to notify users (#6069).
What is the best way to do this?
Is the current plan still to add another user-facing doc somewhere here? (xref: #7672 (comment))
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.
@sbueringer Yes, as suggested we have two release documents: one for users and one for implementers.
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.
The idea with having those documents in the repo was that everyone who implements a change can add the corresponding notes there and it's all transparent
We have good experience with that with the provider implementers doc, e.g. reviewers are regularly asking folks to add the notes in the PRs where they implement the change.
I have some doubts if we can keep this up if we have 2 hackmd docs that are only ~ accessible to the docs team.
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.
(Also this way providers adopting early CAPI versions from main can see what they have to do and modify their providers according to the latest changes to the providers doc, same for users)
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.
You are absolutely right Stefan. I didn't reflect sufficiently on the question asked. What you suggested is a much better solution.
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.
No worries, good that we have discussions like this 😀
LGTM label has been added. Git tree hash: 34d6dfd2569ef5eed154a46d9c88d3fca6a4bfcd
|
cc @chrischdi Just fyi, given you recently updated the Runtime Extensions doc. |
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
/retest |
/approve given lgtm's above. @ykakarap Can we also close #6069 and carry over #7955 (comment) into #7672? This would allow us to close the 1.26 issue. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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 |
+1. Added an item in the release tracking issue for the Communications manager to add a user facing doc and also referenced this issue under that item. cc @oscr |
Signed-off-by: Stefan Büringer [email protected]
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Related: #6069