Organize DEVELOPMENT.md, provide consistent examples & reflect go mod support #3955

mrutkows · 2021-05-17T19:42:18Z

Changes

Organize DEVELOPMENT.md, provide consistent examples and reflect go mod support

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Docs included if any changes are user facing
N/A Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

Organized `DEVELOPMENT.md` to provide more consistent examples and instructions.

tekton-robot · 2021-05-17T19:42:32Z

Hi @mrutkows. Thanks for your PR.

I'm waiting for a tektoncd 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.

pritidesai · 2021-05-17T20:15:28Z

/ok-to-test

ghost

@afrittoli also has an extremely useful script for getting a Kind cluster installed with the latest version tekton pipelines running inside it.

I wonder if something like that would be useful as an "extremely quick quickstart"? We could place in contrib/ and reference it from here. Not a blocker on this PR though at all.

ghost · 2021-05-18T12:34:32Z

DEVELOPMENT.md

+
+The recommended configuration is:
+
+-  Kubernetes version 1.17 or later


This has changed very recently (pipelines 0.24) to "1.18 or later".

ghost · 2021-05-18T12:40:44Z

DEVELOPMENT.md

+The recommended configuration is:
+
+-  Kubernetes version 1.17 or later
+-  4 vCPU nodes (`n1-standard-4`)


I wonder if we should be pointing developers at Kind, instead of cloud resources? Might be a lower barrier given it's free?

If we stick with machine recommendations, maybe change to e2-standard-2? It's cheaper and, at least during my development, Tekton Pipelines alone hasn't required lots of CPU / RAM resources.

Ah, I also notice we have GKE-specific instructions below. Maybe move the specific machine recommendation (which is a Google Cloud designation I think?) to that section instead?

Ah, I also notice we have GKE-specific instructions below. Maybe move the specific machine recommendation (which is a Google Cloud designation I think?) to that section instead?

yes, I agree that referencing pre-packaged, named tiers for specific providers should not be part of the canonical documentation. Not sure when n1-standard-4 was added, but likely should remove altogether.

yup, I agree, these recommendations might confuse developers compared to the machine specific recommendations. @mrutkows please feel free to delete these from here or can be addressed in a follow up PR.

DEVELOPMENT.md

afrittoli · 2021-05-18T13:12:19Z

@afrittoli also has an extremely useful script for getting a Kind cluster installed with the latest version tekton pipelines running inside it.

Indeed, I find kind to be great for development. With KO_DOCKER_REPO=kind.local the images built by ko are uploaded directly into the containerd cache, which is very convenient.

The script is in the plumbing repo.
I also have an evolution of that which includes setting up an ingress controller and more, but I need to clean it up before I can put it back into plumbing.

I wonder if something like that would be useful as an "extremely quick quickstart"? We could place in contrib/ and reference it from here. Not a blocker on this PR though at all.

Agreed, linking the kind script in here could be an extra PR on top.

mrutkows · 2021-05-18T19:02:20Z

@afrittoli also has an extremely useful script for getting a Kind cluster installed with the latest version tekton pipelines running inside it.

Indeed, I find kind to be great for development. With KO_DOCKER_REPO=kind.local the images built by ko are uploaded directly into the containerd cache, which is very convenient.

The script is in the plumbing repo.
I also have an evolution of that which includes setting up an ingress controller and more, but I need to clean it up before I can put it back into plumbing.

I wonder if something like that would be useful as an "extremely quick quickstart"? We could place in contrib/ and reference it from here. Not a blocker on this PR though at all.

Agreed, linking the kind script in here could be an extra PR on top.

Thanks Andrea, would love to integrate anything that would help developers here; if you like, I can add these in follow-on PRs and make sure I gather all the information you are referencing properly.

pritidesai · 2021-05-19T05:51:41Z

thanks a bunch @mrutkows for all the updates, appreciate all your efforts 🙏

pritidesai · 2021-05-19T05:55:17Z

DEVELOPMENT.md


-# Set your target namespace here
-TARGET_NAMESPACE=new-target-namespace
+    **Setting cluster admin role example**:


is this something we can drop from here? or at least delay it until machine specific recommendations 🤔

I reviewed the changes again and this does not apply here unless I am missing something. Not blocking this PR though, this can be revisited in later iteration.

ghost · 2021-05-19T16:19:04Z

integration tests should be back in working order once #3960 is merged. You'll need to rebase once that's in.

nikhil-thomas · 2021-05-20T13:15:49Z

/retest

mrutkows · 2021-05-21T17:17:42Z

Agreed, linking the kind script in here could be an extra PR on top.

Opened #3970 to assure we add kind docs as a follow-on PR and volunteered to learn/document...

…o mod support

mrutkows · 2021-05-21T19:18:54Z

Please review (rebased, squashed commits). I have completed all the changes I intended for the scope of this PR and linked in additional work on kind on an issue I intend to work on.

mrutkows · 2021-05-21T21:38:18Z

Not sure why tests/checks say "failed".. clicking on their "details"

Unit says "SUCCESS"
- https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_pipeline/3955/pull-tekton-pipeline-unit-tests/1395821088566415360/
and the integration says "Not finished"
- https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_pipeline/3955/pull-tekton-pipeline-alpha-integration-tests/1395821088566415364/

pritidesai · 2021-05-21T23:47:37Z

/test pull-tekton-pipeline-alpha-integration-tests
/test tekton-pipeline-unit-tests

pritidesai · 2021-05-22T05:22:41Z

Not sure why tests/checks say "failed".. clicking on their "details"

@mrutkows just a flaky run 😜 runs fine now

ghost

Awesome, thanks a lot!

ghost · 2021-05-24T12:24:43Z

DEVELOPMENT.md


-As you make changes to the code, you can redeploy your controller with:
+You can verify your development installation using `ko` was successful by checking to see oif the Tekton pipeline pods are running in Kubernetes:


nit: oif -> if

ghost · 2021-05-24T12:28:28Z

DEVELOPMENT.md

+
+This script will cause `ko` to:
+- Change (resolve) all `namespace` values in K8s configuration files within the `config/` subdirectory to be updated to a name of your choosing.
+- Builds and push images with the new namespace to your container registry and


nit: Builds -> Build to match tense with bullets above/below

tekton-robot · 2021-05-24T12:29:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

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:

~~OWNERS~~ [sbwsg]

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

pritidesai · 2021-05-24T18:08:29Z

DEVELOPMENT.md

-  --docker-email=me@here.com \
-  --namespace=tekton-pipelines
-```
+    > **NOTE** You may need to set `GOROOT` if you installed Go tools to a a non-default location or have multiple Go versions installed.


NIT: to a a non-default, extra a here

pritidesai · 2021-05-24T18:21:55Z

/lgtm

thanks a bunch @mrutkows for revamping the DEVELOPMENT.md, looks very detailed with easy to follow instructions. There are some NITs left in this PR, please try to address them in a follow up PR. 🙏

Also, I have added appropriate release notes 😄

tekton-robot requested review from dibyom and pritidesai May 17, 2021 19:42

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 17, 2021

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 17, 2021

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 17, 2021

ghost reviewed May 18, 2021

View reviewed changes

pritidesai reviewed May 19, 2021

View reviewed changes

mrutkows mentioned this pull request May 21, 2021

Document use of kind tool for developer k8s cluster deployments #3970

Closed

Organize DEVELOPMENT.md, provide consistent platform info & reflect g…

d67eb65

…o mod support

mrutkows force-pushed the contrib branch from 12cc3b7 to d67eb65 Compare May 21, 2021 19:17

mrutkows marked this pull request as ready for review May 21, 2021 19:17

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 21, 2021

ghost approved these changes May 24, 2021

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 24, 2021

pritidesai reviewed May 24, 2021

View reviewed changes

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 24, 2021

tekton-robot assigned pritidesai May 24, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 24, 2021

tekton-robot merged commit 4ced3cb into tektoncd:main May 24, 2021

mrutkows deleted the contrib branch May 24, 2021 20:14

mrutkows mentioned this pull request May 25, 2021

Document how to use the kind tool and the Tekton/plumbing convenience script #3972

Merged

4 tasks

pritidesai mentioned this pull request May 25, 2021

adding mrutkows to the org tektoncd/community#431

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Organize DEVELOPMENT.md, provide consistent examples & reflect go mod support #3955

Organize DEVELOPMENT.md, provide consistent examples & reflect go mod support #3955

mrutkows commented May 17, 2021 •

edited by pritidesai

Loading

tekton-robot commented May 17, 2021

pritidesai commented May 17, 2021

ghost left a comment

ghost May 18, 2021

ghost May 18, 2021

ghost May 18, 2021

mrutkows May 18, 2021

pritidesai May 19, 2021

afrittoli commented May 18, 2021

mrutkows commented May 18, 2021

pritidesai commented May 19, 2021

pritidesai May 19, 2021

pritidesai May 24, 2021

ghost commented May 19, 2021

nikhil-thomas commented May 20, 2021

mrutkows commented May 21, 2021

mrutkows commented May 21, 2021 •

edited

Loading

mrutkows commented May 21, 2021 •

edited

Loading

pritidesai commented May 21, 2021

pritidesai commented May 22, 2021

ghost left a comment

ghost May 24, 2021

ghost May 24, 2021

tekton-robot commented May 24, 2021

pritidesai May 24, 2021

pritidesai commented May 24, 2021 •

edited

Loading


		The recommended configuration is:

		- Kubernetes version 1.17 or later


		As you make changes to the code, you can redeploy your controller with:
		You can verify your development installation using `ko` was successful by checking to see oif the Tekton pipeline pods are running in Kubernetes:

Organize DEVELOPMENT.md, provide consistent examples & reflect go mod support #3955

Organize DEVELOPMENT.md, provide consistent examples & reflect go mod support #3955

Conversation

mrutkows commented May 17, 2021 • edited by pritidesai Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented May 17, 2021

pritidesai commented May 17, 2021

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afrittoli commented May 18, 2021

mrutkows commented May 18, 2021

pritidesai commented May 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented May 19, 2021

nikhil-thomas commented May 20, 2021

mrutkows commented May 21, 2021

mrutkows commented May 21, 2021 • edited Loading

mrutkows commented May 21, 2021 • edited Loading

pritidesai commented May 21, 2021

pritidesai commented May 22, 2021

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented May 24, 2021

Choose a reason for hiding this comment

pritidesai commented May 24, 2021 • edited Loading

mrutkows commented May 17, 2021 •

edited by pritidesai

Loading

mrutkows commented May 21, 2021 •

edited

Loading

mrutkows commented May 21, 2021 •

edited

Loading

pritidesai commented May 24, 2021 •

edited

Loading