-
Notifications
You must be signed in to change notification settings - Fork 101
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
https://issues.redhat.com/browse/ACM-15420 #7277
Conversation
---- | ||
<1> Define the global settings for all managed clusters that have the observability add-on enabled. | ||
<2> Used to indicate if the observability add-on is enabled to push metrics to hub cluster server. |
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.
If I remember correctly we aren't supposed to use callouts for additional information, they are only for variable replacements.
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.
@oafischer The additional information here is used to explain why the parameters are used. Since last release, I have used callouts as a solution to remove hashtags out of YAMLs.
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'm OK with the suggestions you provide further down but we might need to bring this up in a meeting since I've been tasked with removing callouts that aren't about replacing variables in the past. Thanks for finding a compromise in the meantime. I'll approve after merging the suggestions.
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.
@oafischer ok cool, we can discuss in the team channel before I merge.
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.
This is I think not something we need to stress over, as I see teams use these callouts for a number of things now that I look at it. I don't agree with overusage, or even some of the writing here--or using two at one time for the same line, but see that they are used here by the telco/ocp writer:
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.
Maybe when we first started using them, because we didn't before, we had guidance to only use them for variables but I lean towards using them to replace any #comments-that-clutter-the-code.
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.
See here where the same product doc isn't clean and the code is cluttered with comments:
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.
But I will say that since you have punctuation, these need to be full sentences.
Used to indicate if the observability add-on is enabled to push metrics to hub cluster server. (not complete)
Also, we agreed that unless in code, Observability is proper. You can do either way here. :-)
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.
@swopebe thanks for your comments and examples. I have updated the file with complete sentence and capitalized "observability" where needed. Awaiting review from @saswatamcode
Co-authored-by: Mikela Jackson <[email protected]>
Co-authored-by: Mikela Jackson <[email protected]>
Co-authored-by: Mikela Jackson <[email protected]>
Updating the capitalization for Observability and replaced ROSA reference with the conref.
@oafischer @saswatamcode thanks for yalls reviews. I made a change based on @saswatamcode suggestion. When you both have an opportunity, please re-review this PR |
@oafischer @saswatamcode commit made with some adjustments |
/lgtm |
---- | ||
<1> Use the `observabilityAddonSpec` parameter to define the global settings for all managed clusters that have the Observability add-on enabled. | ||
<2> Use the `enableMetrics` parameter to indicate that the Observability add-on is enabled to push metrics to hub cluster server. | ||
<3> Use the `workers` parameter to increase the number of internal workers in the metric collector process. The internal workers shard `/federate` endpoint requests made to Prometheus on your managed cluster, to the internal workers, and sends separate remote-write requests to Thanos on your hub cluster. |
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.
As far as I understand this feature is in "Tech Preview". Do we want to make a note of this? cc: @saswatamcode
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.
Tech preview does need to be noted in the docs. @dockerymick
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.
Thanks for the reminder! @oafischer @jacobbaungard I just made updates related to the reminder
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.
Looks fine to me but perhaps @saswatamcode who worked on this feature would want to have a look as well?
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dockerymick, jacobbaungard, oafischer, swopebe The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
New changes are detected. LGTM label has been removed. |
Co-authored-by: Saswata Mukherjee <[email protected]>
We agreed that there should be a new issue opened to address the changes related to 2.12.2. My recommendation made through Slack to @saswatamcode: "based on the z-stream excel sheet, It looks like ACM 2.12.2 will be released [in the new year]. I recommend that a new issue be created and the change be addressed in another PR. I will be on leave [soon], so having the issue will be helpful to Oliver, who is helping with the Observability docs (in case I do not get to the update before I leave). Considering the holidays and recharge coming up, I’m not sure that the update will get in before the end of this week. But I should be able to get to it in the New Year." From my perspective, it seems like we can move forward and merge this PR. |
No description provided.