-
Notifications
You must be signed in to change notification settings - Fork 151
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
Refactored the examples and rendered directories into one for better usability, #658
Conversation
…usability, added more examples, updated related CI/CD files
8f1e726
to
324527f
Compare
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.
A lot of random little wording nits, sorry if it's a bit overboard! Thanks for the reorganization!
I only reviewed the first commit, as suggested.
examples/collector-cluster-receiver-only/collector-cluster-receiver-only-values.yaml
Outdated
Show resolved
Hide resolved
examples/route-data-through-gateway-deployed-separately/README.md
Outdated
Show resolved
Hide resolved
examples/route-data-through-gateway-deployed-separately/README.md
Outdated
Show resolved
Hide resolved
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.
Why it's 13k+ changeset? Do we add new examples?
Can you please do that in a follow-up PR and keep this changeset for the refactoring only? |
I was having "make render" render all the examples and place the output k8s manifests into respective subdirectories, that's how this PR got up to +13k. Disabled rendering for now. Several users keep reaching out for specific rendered Helm Chart files created from different configurations at different versions of the chart. So I would like to ease this pain point. |
Sorry, I still don't understand why it's such a big changeset if you didn't add any new examples. The rendered files shouldn't be changed significantly, they just moved around, right? |
LGTM @crobert-1 PTAL |
@crobert-1 the last commit I just added (505ece8)addresses the last of the comments you had. I'm going to add one more commit now that will re-enable rendering and add back the large number of rendered files. |
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 making all of those updates!
This refactor combines the rendered and example directories so they are one. Also added more examples for the most popular use cases.
Current Directory Structure:
Refactor Directory Structure:
When reviewing this PR, I'd suggest only reviewing the first commit because all the new rendered yaml files are added in the second commit.