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

Getting started guide part 2 #4748

Merged

Conversation

rob8714
Copy link
Contributor

@rob8714 rob8714 commented Aug 3, 2022

  • Renamed the overlays from 'Production' and 'Staging' to 'Variant 1' and 'Variant 2' to make it more generic
  • Added further customisations

Contributes to #3973

@k8s-ci-robot
Copy link
Contributor

@rob8714: This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

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.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 3, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @rob8714. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 3, 2022
@k8s-ci-robot k8s-ci-robot requested review from KnVerey and yuwenma August 3, 2022 20:21
@rob8714
Copy link
Contributor Author

rob8714 commented Aug 3, 2022

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Aug 3, 2022
@rob8714
Copy link
Contributor Author

rob8714 commented Aug 3, 2022

/assign @KnVerey

@KnVerey
Copy link
Contributor

KnVerey commented Aug 10, 2022

Thanks for the update! I like the new content, but I'm not sure about the rename of prod/staging to variant1/2. I think it can be helpful to anchor the tutorial in a concrete use case that is a common reason folks initially reach for Kustomize. Are there downsides to this that I am missing?

@KnVerey
Copy link
Contributor

KnVerey commented Aug 10, 2022

Btw, while we're updating this page, I noticed a small inconsistency: some parts of the example use "our" while others use "your". For example, this surfaces in the headings as shown in the sidebar:

@rob8714
Copy link
Contributor Author

rob8714 commented Aug 11, 2022

Thanks for the feedback, I thought about renaming the overlays for 2 reasons:

  • the names became shorter, though I could rename them to use prod and stg instead to keep them short
  • I didn't want this to be taken as a literal example of something to implement in a production environment, though we could put a disclaimer in the example for this.

I'm ok with changing them back, let me know.

Re: inconsistency, now that you mention it I notice it too. I think we could align the titles to use 'your' and keep the text with 'our'. Do you have any preference?

@KnVerey
Copy link
Contributor

KnVerey commented Aug 12, 2022

the names became shorter, though I could rename them to use prod and stg instead to keep them short

I'm not too worried about the length. In a basic example, I think clarity is more important than concision. I'd prefer to go back to "production" and "staging".

I didn't want this to be taken as a literal example of something to implement in a production environment, though we could put a disclaimer in the example for this.

Is there a particular aspect you're worried would be bad to copy? I don't think anyone would infer that we're promoting deploying the Deployment sample, for example, and the Kustomize portions should be fine to copy if they're what you need. Note that the kubernetes.io docs have similar content and don't have any caveats: https://kubernetes.io/docs/tasks/manage-kubernetes-objects/declarative-config/#how-to-create-objects.

Re: inconsistency, now that you mention it I notice it too. I think we could align the titles to use 'your' and keep the text with 'our'. Do you have any preference?

It occurred to me that SIG Docs might have standards for this, and it turns out they do! They specifically say that we SHOULD use "you" and SHOULDN'T use "we". So let's go with that. For the headings, I suggest dropping the pronouns, e.g. "Create resource manifests and Kustomization"/"Customize resources".

One more thing I just noticed: you appear to be using British spelling in this doc. That's another thing SIG Docs has guidance on: we should be using US English (this is a challenge for me too, as a Canadian 😄 ).

@rob8714
Copy link
Contributor Author

rob8714 commented Aug 13, 2022

Is there a particular aspect you're worried would be bad to copy?

Was a bit concerned about the image because it would become outdated eventually, but I see the link you referenced uses an even older image.

I have reverted the overlay name changes and updated the doc to use "you/your" instead of "we/our". I have also updated the places where I was using British spelling, not sure if I missed any (it was a challenge 😄)

@KnVerey
Copy link
Contributor

KnVerey commented Aug 15, 2022

Thanks for making those changes! I have a few final suggestions. Don't worry about the irrelevant lint errors.

@rob8714
Copy link
Contributor Author

rob8714 commented Aug 16, 2022

Thanks, suggestions applied

@KnVerey
Copy link
Contributor

KnVerey commented Aug 16, 2022

/ok-to-test
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 16, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KnVerey, rob8714

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 16, 2022
@k8s-ci-robot k8s-ci-robot merged commit 26fcafd into kubernetes-sigs:master Aug 16, 2022
@rob8714 rob8714 deleted the getting-started-guide-part-2 branch August 16, 2022 18:13
koba1t pushed a commit to koba1t/kustomize that referenced this pull request Sep 3, 2022
* rename overlays

* add further examples in first kustomization

* fix typo

* fix formatting

* fix typo

* fix formatting

* fix typos

* restore overlay names to production and staging in original content

* restore overlay names to production and staging in original content

* restore overlay names to production and staging in new content

* updated doc to use "you/your" vs "we/our", and updated to use US spelling

* fix capitalization

* Update site/content/en/docs/Getting started/first_kustomization.md

Co-authored-by: Katrina Verey <[email protected]>

* Update site/content/en/docs/Getting started/first_kustomization.md

Co-authored-by: Katrina Verey <[email protected]>

* add "patch:" for patches in kustomization, and add url link

* Update site/content/en/docs/Getting started/first_kustomization.md

Co-authored-by: Katrina Verey <[email protected]>

* fix typo

Co-authored-by: Katrina Verey <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants