docs: Add README.md for nginx sample#7179
Conversation
karmada-bot
added
the
kind/documentation
|
Welcome @abhicodes11! It looks like this is your first PR to karmada-io/karmada 🎉 |
Summary of ChangesHello @abhicodes11, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience for the Nginx sample by introducing a detailed Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
karmada-bot
added
the
size/M
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a helpful README.md for the nginx sample, which will greatly improve the onboarding experience for new users. The guide is well-structured and clear. I've found one minor omission in the cleanup instructions that should be addressed to make the example fully complete.
| kubectl delete -f propagationpolicy.yaml | ||
| kubectl delete -f deployment.yaml |
There was a problem hiding this comment.
The cleanup instructions are incomplete. The overridepolicy.yaml is created in the optional step 4, but its deletion is missing from the cleanup section. To ensure a complete cleanup and avoid leaving orphaned resources, the command to delete the OverridePolicy should also be included.
| kubectl delete -f propagationpolicy.yaml | |
| kubectl delete -f deployment.yaml | |
| kubectl delete -f overridepolicy.yaml | |
| kubectl delete -f propagationpolicy.yaml | |
| kubectl delete -f deployment.yaml |
XiShanYongYe-Chang
left a comment
XiShanYongYe-Chang
left a comment
There was a problem hiding this comment.
@abhicodes11 Thanks for your contribution.
The sample folder is just a file for developers to quickly test, but adding a readme file doesn't seem to be a problem either.
/lgtm
For more information about these features, you can refer to the documents in the website repository.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7179 +/- ##
==========================================
- Coverage 42.09% 42.01% -0.09%
==========================================
Files 871 874 +3
Lines 53288 53542 +254
==========================================
+ Hits 22434 22494 +60
- Misses 29168 29363 +195
+ Partials 1686 1685 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@zhzhuang-zju You're right! The Multi-Cluster Service link isn't relevant since this example only shows propagation and override policies. I've removed it to keep the documentation focused on what's actually demonstrated. Thanks for the feedback! |
There was a problem hiding this comment.
Pull request overview
Adds a new README for the samples/nginx example to help users deploy nginx across member clusters with Karmada.
Changes:
- Add prerequisites and environment verification steps.
- Document the sample manifests and provide step-by-step deployment/verification instructions.
- Add cleanup steps and links to relevant Karmada docs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| Before running this example, ensure you have: | ||
| - A Karmada control plane running (see [Quick Start](../../README.md#quick-start)) | ||
| - At least one member cluster joined to Karmada |
There was a problem hiding this comment.
The prerequisites say “at least one member cluster”, but this sample’s propagationpolicy.yaml explicitly targets member1 and member2. As written, the steps will not work on a single member cluster; either update prerequisites to require two joined clusters (named member1/member2) or adjust the policy/steps to match a single-cluster setup.
| - At least one member cluster joined to Karmada | |
| - Two member clusters (`member1` and `member2`) joined to Karmada |
| ## Step-by-Step Guide | ||
|
|
||
| ### 1. Deploy the nginx application | ||
| ```bash | ||
| kubectl create -f deployment.yaml | ||
| ``` |
There was a problem hiding this comment.
The commands use relative filenames (e.g., deployment.yaml) but the README never tells the reader to cd samples/nginx first. This will fail if someone runs the instructions from the repo root (as the main README does). Add an explicit cd step near the top or switch the commands to use samples/nginx/... paths consistently.
| export KUBECONFIG=$HOME/.kube/karmada.config | ||
| kubectl config use-context karmada-apiserver | ||
|
|
||
| # Delete resources |
There was a problem hiding this comment.
If the optional overridepolicy.yaml is applied, the cleanup instructions should also delete it; otherwise the sample leaves an OverridePolicy behind in the Karmada control plane. Consider adding kubectl delete -f overridepolicy.yaml (or noting it’s only needed if step 4 was run).
| # Delete resources | |
| # Delete resources (include overridepolicy.yaml if you applied step 4) | |
| kubectl delete -f overridepolicy.yaml |
| ```bash | ||
| # Verify you're connected to Karmada | ||
| kubectl config use-context karmada-apiserver | ||
|
|
||
| # Check member clusters are ready | ||
| kubectl get clusters | ||
| ``` |
There was a problem hiding this comment.
The verification snippet switches context to karmada-apiserver but doesn’t show setting KUBECONFIG to the Karmada kubeconfig (e.g., $HOME/.kube/karmada.config). Other samples (and the root Quick Start) include this, and without it the context may not exist in the user’s default kubeconfig.
|
Hi @abhicodes11 can you help squash the commits into one? |
|
Kindly ping @abhicodes11 |
abhicodes11
force-pushed
the
add-nginx-readme
branch
from
22fa657 to
d9a5d95
Compare
|
Hi @XiShanYongYe-Chang, apologies for the delay. I’ve squashed the commits into a single commit and force-pushed the branch. Please let me know if anything else needs to be updated. |
|
Thanks @abhicodes11 Leave lgtm to @zhzhuang-zju make confirm. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: XiShanYongYe-Chang The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
karmada-bot
added
the
approved
|
File download timed out. /retest |
samples/nginx/README.md
Outdated
|
|
||
| ## Learn More | ||
|
|
||
| - [Propagation Policy Documentation](https://karmada.io/docs/userguide/scheduling/resource-propagating) |
There was a problem hiding this comment.
| - [Propagation Policy Documentation](https://karmada.io/docs/userguide/scheduling/resource-propagating) | |
| - [Propagation Policy Documentation](https://karmada.io/docs/userguide/scheduling/propagation-policy) |
This link will be deprecated after the release of release‑1.17.
We can wait until release 1.17 is published, and then merge the change.
|
kindly ping @abhicodes11 |
abhicodes11
force-pushed
the
add-nginx-readme
branch
from
d9a5d95 to
122b41e
Compare
Signed-off-by: Abhi <abhicodes11@users.noreply.github.com> Remove irrelevant Multi-Cluster Service link Signed-off-by: Abhi <abhicodes11@users.noreply.github.com>
abhicodes11
force-pushed
the
add-nginx-readme
branch
from
122b41e to
51d5c89
Compare
|
@zhzhuang-zju @XiShanYongYe-Chang Apologies for the delay - I've updated the deprecated documentation link as requested. The PR should now address all feedback. Thanks for your patience! |
|
Thanks @abhicodes11 No need to apologize. |
|
Thanks, I have no further comments. Once karmada-io/website#987 is merged, we can merge this PR too. |
|
/retest |
|
/lgtm |
What type of PR is this?
/kind documentation
What this PR does / why we need it:
Adds a comprehensive README to the nginx sample to help new users understand and run the example step-by-step.
The README includes:
Which issue(s) this PR fixes:
N/A (documentation enhancement)
Why this matters:
The nginx sample is often the first example new users try. A clear README reduces confusion and improves the onboarding experience.
Special notes for reviewers:
I tested these steps on a fresh Karmada installation to ensure accuracy.