-
Notifications
You must be signed in to change notification settings - Fork 223
feat: support setting loggingService and monitoringService #1292
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
Conversation
|
Welcome @darkweaver87! |
|
Hi @darkweaver87. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
✅ Deploy Preview for kubernetes-sigs-cluster-api-gcp ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
5c4c206 to
3f9485b
Compare
|
/ok-to-test |
|
Thanks for the PR @darkweaver87. Looks like you need to fix some linting issues and run |
|
@salasberryfin thank you for guiding me on this and sorry :-) |
|
@darkweaver87 - thanks for this change. Would you be able to change the commit messages so they don't include the |
| // +optional | ||
| MasterAuthorizedNetworksConfig *MasterAuthorizedNetworksConfig `json:"master_authorized_networks_config,omitempty"` | ||
| // LoggingService represents configuration of logging service feature of the GKE cluster. | ||
| // Possible values: none, logging.googleapis.com/kubernetes (default for GKE 1.14+), logging.googleapis.com (default for GKE < 1.14). |
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.
Should we have some validation for this? Or shall we assume it's a cluster greater than 1.14 and so the only values allowed would be:
- none
- logging.googleapis.com/kubernetes
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.
IIUC the google documentation we can't use logging.googleapis.com for GKE > 1.14 so yes it makes sense. I'll update the PR.
Thanks.
5d82a2d to
d286584
Compare
e2c188d to
fc2e057
Compare
|
Thaks @darkweaver87, this looks good to me. Before we merge would you be able to squash your commits? |
|
@richardcase , yes surely. Thanks :-) |
fc2e057 to
3e1996f
Compare
|
/lgtm |
|
@richardcase You said it looked good to you. Would you please do the final approve on this one ? |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
|
/remove-lifecycle stale |
|
@richardcase @cpanato Friendly reminder for those who can approve. This PR is quite straightforward. |
cpanato
left a 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.
seems good
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cpanato, darkweaver87 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 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
On GKE, loggingService and monitoringService are enabled by default. Some people might don't need it as they have their own observability stack. Moreover, the installed services are not working on ARM architectures.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
I'm not sure if I should add unit tests and where ?
gcpmanagedcontrolplane_webhookdon't have tests, should I create it ?TODOs:
Release note: