-
Notifications
You must be signed in to change notification settings - Fork 180
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
Fix quickstart links #2293
Fix quickstart links #2293
Conversation
@grzywin - I'm unsure if it fixes the problem, but I hope the links are correct now. Perhaps we need fully hyperlink to the documentation in docs.scylladb.com? @annastuchlik ? |
e2326d2
to
3afc882
Compare
@mykaul It looks good now. I checked your PR and links are working - except "Contributing Guide" which you are aware of and also one Slack channel link needs to be updated (I wrote about it in the code review). |
@mykaul @annastuchlik IMHO, it could also be beneficial for the clients to include a link to the basic-setup URL in the README.md file. It provides a great explanation of how the basic configuration of the Operator and Scylla should look. The only thing that could be tweaked there is information about requirements regarding initial K8s configuration and some extra comment about NodeConfig configuration as the example in run.sh is doing it in GKE and we have also examples for EKS and Generic. |
It defeats the entire effort of the docs explaining the architecture and different configuration options. With the recording people are just going to run the script. As you correctly pointed out it's also platform specific. |
But this is unrelated to this PR? |
Yes, this is unrelated (it is about basic-setup script), I just thought it is worth to mention about this here.
Well, you are right – the Operator is intended for more experienced users who understand the architecture, so the documentation should be the main source of information. However, if someone wants to experiment a bit, this shortcut to the basic setup could make things easier for them. Anyway, it was just a loose thought – you guys know the audience better than I do. |
IMO yes, it's a better experience than just opening the markdown file in github. @mykaul can you also please update the PR title and commit subject line to use the imperative mood? https://cbea.ms/git-commit/#imperative (it should be mentioned in the contribution guide which we currently don't have) |
Once we have the guidelines, I'll improve it. See scylladb/scylladb#22214 for a very similar situation (scylladb/scylladb#22139 (comment) ) And btw, 'fix' is imperative. |
cc @zimnx, we should pick up #2130
You're right, I should've said it shouldn't have the prefix. We should include that in the guide as well. |
SCT requires some prefix, Scylla requires a module, here I don't need a prefix at all... Oh boy. |
The links are correct, but it would be better to add links to the published documentation: GKE quickstart: https://operator.docs.scylladb.com/stable/quickstarts/gke.html Installation generic: https://operator.docs.scylladb.com/stable/installation/kubernetes/generic.html Releases: https://operator.docs.scylladb.com/stable/support/releases.html |
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
/kind cleanup
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mflendrich, mykaul 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 |
/override e2e-gke-multi-datacenter-parallel |
@mflendrich: mflendrich unauthorized: /override is restricted to Repo administrators. In response to this:
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. |
/test e2e-gke-multi-datacenter-parallel |
/hold to sort out the changelog autogeneration before merge |
/test e2e-gke-multi-datacenter-parallel |
@mykaul: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
c4a0e97
to
8f0169c
Compare
Fixes: scylladb#2270 In addition, renamed Scylla to ScyllaDB when needed. Signed-off-by: Yaniv Kaul <[email protected]> Co-Authored-By: Michal Flendrich <[email protected]>
8f0169c
to
14a3865
Compare
/hold cancel |
Fixes: #2270
Description of your changes:
Fixes README links to quickstarts
Which issue is resolved by this Pull Request:
Resolves 2270