-
Notifications
You must be signed in to change notification settings - Fork 302
o11y: Split MonitoringStack by environment #7509
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
cc @gcpsoares @mftb |
/lgtm |
Approved, I am just wondering for what is the With that its just the question if pipeline of adding something should be just: update |
Yup, that directory is for deployment to development clusters (e.g. local dev cluster)
Yup. Changes should be made to EDIT: nvm, looks like we need do that another way. I get this error if I try to directly reference the staging MonitoringStack defintion:
|
ERROR: |
I kept the old base MonitoringStack to be referred to by |
ERROR: |
The outcome of just the staging/developments pointing to the base while production points to one outside the base effectively inverts the expectation that "base is Production and anything that shares its config" - instead of a follow-up PR, could we include the change also in this PR (so both Development and Stage point to the same config in their own directory, and not to the base)? |
I am thinking if we can just create a "symlink" to the file in staging perhaps? with |
Using symlinks, I get the same error I posted previously. |
@ci-operator @TominoFTW - Revising this but any idea on how to go about this? I cannot have development's kustomization.yaml refer directly to resources outside of its directory (e.g. An idea I have is using the following structure but what do you guys think?
|
Hmm, I am worried that it gets more and more confusing with that change 🤔 But I don't really see any other way out of that tho, so if that is working I am okay with using that. 👍 |
Or better yet:
|
One more thought for this:
Could something like this be done? |
I think this runs into the same problem. It could be assumed that |
then I just want to completely omit the idea of having nested folders when symlinks don't work 🤔 |
ERROR: |
I agree with avoiding nested folders. Made a new folder for holding staging and development common resources and moved the MonitoringStack definition under there. |
03412ad
to
b8046b8
Compare
Code Review by GeminiThe changes aim to split the However, there are a couple of points to consider: Duplication of MonitoringStack DefinitionThe This duplication means that any future changes to the core Suggestion: This approach would reduce duplication and make it easier to manage common configurations while still allowing for environment-specific overrides. Resource Allocation for Staging/DevelopmentIn # components/monitoring/prometheus/stg-dev-common/monitoringstack/monitoringstack.yaml
requests:
cpu: 500m
memory: 16Gi
limits:
memory: 16Gi While this might be appropriate for production, Suggestion: |
ERROR: |
6abeb36
to
9bf4a12
Compare
Code Review by GeminiThe changes aim to split the Identified Issues and Suggested Changes:1. File:
|
This is in attempt to speed up the pipeline whenever changes to this MonitoringStack definition is made. This is especially crucial for if we need to revert a breaking change to this definition. Additional, this allows us to test changes to this definition in dev or stage first before applying them to production.
9bf4a12
to
6c8d608
Compare
Code Review by GeminiThe changes aim to split the Issues and Suggestions1. Bug: Incorrect Kustomize operation in
|
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
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ci-operator, pacho-rh, TominoFTW 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 |
This is in attempt to speed up the pipeline whenever changes to this MonitoringStack definition is made. This is especially crucial for if we need to revert a breaking change to this definition. Additional, this allows us to test changes to this definition in dev or stage first before applying them to production.