Skip to content
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

Remove Master/Slave terminology from stateless application tutorial. #22934

Conversation

paulczar
Copy link
Contributor

@paulczar paulczar commented Aug 3, 2020

The stateless application tutorial is littered with unsuitable terminology. This update switches the database technology to a database that has already updated their own terminology as well as removes the terminology from the tutorial itself.

The advanced logging tutorial followup is set to Draft as it will take a larger effort to convert that, and I'm not convinced its even in a working state right now.

A followup PR to the examples repo is here - kubernetes/examples#393

Addresses #22918 #19821 and #20088

Signed-off-by: Paul Czarkowski username.taken@gmail.com

The stateless application tutorial is littered with unsuitable terminology. This update switches the database technology to a database that has already updated their own terminology as well as removes the terminology from the tutorial itself.

The advanced logging tutorial followup is set to Draft as it will take a larger effort to convert that, and I'm not convinced its even in a working state right now.

A followup PR to the examples repo to be referenced here will be made.

Addresses kubernetes#22918

Signed-off-by: Paul Czarkowski <username.taken@gmail.com>
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 3, 2020
@k8s-ci-robot k8s-ci-robot requested review from onlydole and sftim August 3, 2020 19:57
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Aug 3, 2020
paulczar added a commit to paulczar/examples that referenced this pull request Aug 3, 2020
The guestbook and redis documentation are littered with master/slave terminology. This update along with the update to the [website](kubernetes/website#22934) removes that terminology and technology.

Addresses kubernetes/website#22918

Signed-off-by: Paul Czarkowski <username.taken@gmail.com>
@paulczar
Copy link
Contributor Author

paulczar commented Aug 3, 2020

We need a more appropriate place to host the built docker image as I don't have access to the original google repository, and it shouldn't be hosted in my personal one.

@sftim
Copy link
Contributor

sftim commented Aug 3, 2020

This looks like a work-in-progress (not yet ready for review) - is that right @paulczar ?

@sftim
Copy link
Contributor

sftim commented Aug 3, 2020

BTW, Redis uses the term “replica” in place of the problematic word “slave”.

@paulczar
Copy link
Contributor Author

paulczar commented Aug 3, 2020

@sftim I would call it ready for review. There's only really the outstanding issue of needing a forever home for the image which I'll work with @castrojo on, otherwise review away.

@netlify
Copy link

netlify bot commented Aug 3, 2020

Deploy preview for kubernetes-io-master-staging ready!

Built with commit f7a4acf

https://deploy-preview-22934--kubernetes-io-master-staging.netlify.app

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried that the removal will break existing links to the Redis manifests. I'd like to make sure that these still work or that we've made sure they aren't referenced from elsewhere.

/hold

waiting for the revised container image to be hosted as part of the Kubernetes project

- name: php-redis
image: gcr.io/google-samples/gb-frontend:v4
- name: guestbook
image: paulczar/gb-frontend:v5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does need to change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also these manifest changes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems like the correct image would be image: gcr.io/google-samples/gb-frontend:v5. image version was updated here https://console.cloud.google.com/gcr/images/google-samples/GLOBAL/gb-frontend around the same time of this initial PR

Copy link
Contributor

@kbhawkey kbhawkey Jan 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@notchairmk, OK. Is this image publicly available?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kbhawkey yes, publicly available. Although, gcr.io/google-samples/gb-frontend:v5 seems more tied to redis, but relies on a slightly different configuration.

I built the guestbook app from Paul's dockerfile PR'd against kubernetes/examples. Unfortunately with that image the guestbook app doesn't appear to run correctly (specifically I'm currently experiencing data duplication on submit).

For both of these reasons, it might be preferable to stay with redis gcr.io/google-samples/gb-frontend:v4 and update file/naming terminology to leader/follow.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 3, 2020
Signed-off-by: Paul Czarkowski <username.taken@gmail.com>
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 3, 2020
Signed-off-by: Paul Czarkowski <username.taken@gmail.com>
@celestehorgan
Copy link
Contributor

/wg naming

@k8s-ci-robot k8s-ci-robot added the wg/naming Categorizes an issue or PR as relevant to WG Naming. label Aug 5, 2020
@justaugustus
Copy link
Member

/assign
(request from @castrojo)

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2020
@k8s-ci-robot
Copy link
Contributor

@paulczar: PR needs rebase.

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/test-infra repository.

@castrojo
Copy link
Member

@justaugustus Any word on where can host that container image?

@justaugustus
Copy link
Member

@ameukam merged a PR to create a staging repository which can house these images: kubernetes/k8s.io#1299 (ref: #22515, kubernetes/k8s.io#1033).

It sounds like the next bit would be wiring up the k/examples repo to do image building and support PRs like kubernetes/examples#393 (ref: #22918).

@ameukam
Copy link
Member

ameukam commented Oct 12, 2020

We can also setup a dedicated staging container registry for those images. I am happy to help if there is any need!

@paulczar
Copy link
Contributor Author

I'd love to take some help if people are offering, I don't have a lot of time over the next few weeks to work on this.

@paulczar paulczar force-pushed the remove-redis-and-master-slave-terminology branch from bfaf512 to f7a4acf Compare October 13, 2020 21:21
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign xiangpengzhao after the PR has been reviewed.
You can assign the PR to them by writing /assign @xiangpengzhao in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kbhawkey
Copy link
Contributor

Hi @paulczar . Just checking on the status of the pull request?
Do you still plan to replace Redis with MongoDB?
What work is remaining to get this PR closer to be ready for review?

@paulczar
Copy link
Contributor Author

hey @kbhawkey I probably need to take some time to implement @justaugustus suggestions above, This week is already toast for me, hopefully next week I can focus on this for a day and get it closed out.

@kbhawkey
Copy link
Contributor

hey @kbhawkey I probably need to take some time to implement @justaugustus suggestions above, This week is already toast for me, hopefully next week I can focus on this for a day and get it closed out.

Thanks for the update!

@justaugustus
Copy link
Member

I'm going to look into some of this this week.

@onlydole
Copy link
Member

onlydole commented Nov 5, 2020

Thank you, @justaugustus! I'll be free next week if you need anything extra on this front.

@sftim
Copy link
Contributor

sftim commented Dec 20, 2020

Some extra context: #25704

@bdwill
Copy link

bdwill commented Jan 5, 2021

I went through the tutorial and it worked well. I have a few comments:

Creating the Mongo Deployment

  • Step 3 listed as step 1
  • Step 4 listed as step 2. In this step, I suggest commenting why this step is being performed or add (optional)

Creating the MongoDB Service

  • Step 2 listed as step 1
  • Output from kubectl get service needs to be updated to reflect port 27017 for mongodb

Creating the Guestbook Frontend Deployment

  • Step 2 listed as step 1
  • Step 2 command needs to be updated to reflect correct labels: kubectl get pods -l app.kubernetes.io/component=frontend -l app.kubernetes.io/name=guestbook

Creating the Frontend Service

  • Word missing in last sentence. Correct: "However as a Kubernetes user you can use kubectl port-forward to access the service even though it uses a ClusterIP."
  • Step 2 listed as step 1
  • Update output from kubectl get services to reflect correct mongodb port 27017

Viewing the Frontend Service via kubectl port-forward

  • Indicate that kubectl port-forward should be performed in a separate window or exited with control+C to complete the rest of the tutorial

@TheKLARKEN
Copy link

I have a few notes for things I've noticed:

  • I’m not sure if this is the preview site's fault, because this happens all over the site, but clicking the “copy to clipboard” in the preview (link) copies not just the YAML but also the name and path next to the copy button. The current docs don't do this, just the preview site. This behavior may cause some users to be confused by YAML parsing errors like line 3: mapping values are not allowed in this context, because they aren’t aware that they need to remove the first line that gets pasted.
  • The numbers of the steps don’t go 1, 2, 3, etc. like they should, they go 1, 1, 1, etc. or 1, 2, 1, 2, etc. in some places.
  • kubectl get pods -l app=guestbook -l tier=frontend doesn’t return the correct result, because the pods are labeled like this:
    • labels:
        app.kubernetes.io/name: guestbook
        app.kubernetes.io/component: frontend
  • In “Viewing the Frontend Service via LoadBalancer,” TYPE should return LoadBalancer, not ClusterIP.
  • In “Cleaning up,” users should probably also check services, not just pods, to show that everything is deleted (e.g. ‘kubectl get svc,po’).

Everything else looked good. 🙂

@paulczar
Copy link
Contributor Author

paulczar commented Jan 8, 2021

thanks folks for the reviews, I'm waiting to hear back on image storage before I get back to this, but rest assured its not forgotten.

@kbhawkey
Copy link
Contributor

Hi @paulczar .
Since PR #26141 merged, can this PR close?

@justaugustus
Copy link
Member

Since PR #26141 merged, can this PR close?

Yep!
/close

@k8s-ci-robot
Copy link
Contributor

@justaugustus: Closed this PR.

In response to this:

Since PR #26141 merged, can this PR close?

Yep!
/close

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/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. language/en Issues or PRs related to English language needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. wg/naming Categorizes an issue or PR as relevant to WG Naming.
Projects
None yet
Development

Successfully merging this pull request may close these issues.