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

update development guide #654

Merged
merged 3 commits into from
Sep 7, 2021
Merged

update development guide #654

merged 3 commits into from
Sep 7, 2021

Conversation

cjerad
Copy link
Contributor

@cjerad cjerad commented Sep 2, 2021

Issue, if available:

Description of problem(s):

  1. make codegen fails, even after running make toolchain, because of missing executables
  2. make apply fails because cluster namespace, "karpenter", does not exist

Description of changes:

  1. Add instruction to update PATH with Go workspace's bin after running make toolchain
  2. Add instruction to create namespace, "karpenter"

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@netlify
Copy link

netlify bot commented Sep 2, 2021

✔️ Deploy Preview for karpenter-docs-prod canceled.

🔨 Explore the source changes: 1f0e713

🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/61377f36c73c1a00078a63d4

@@ -13,7 +13,7 @@ The following tools are required for contributing to the Karpenter project.
| [go](https://golang.org/dl/) | v1.15.3+ | [Instructions](https://golang.org/doc/install) |
| [kubectl](https://kubernetes.io/docs/tasks/tools/install-kubectl/) | | `brew install kubectl` |
| [helm](https://helm.sh/docs/intro/install/) | | `brew install helm` |
| Other tools | | `make toolchain` |
| Other tools | | `make toolchain` then ensure `PATH` contains Go workspace's `bin` |
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of putting this here, what do you think about adding some output in the toolchain.sh script to echo something like: PATH=$PATH:$GOPATH/bin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🆗

@@ -39,6 +39,7 @@ make delete # Uninstall Karpenter
### Build and Deploy
```
make dev # build and test code
kubectl create namespace karpenter # create target namespace for deployment
Copy link
Contributor

@ellistarn ellistarn Sep 2, 2021

Choose a reason for hiding this comment

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

Nice catch. We typically have this namespace created as part of the getting-started guide (helm install --create-namespace), but this is a great note to have here.

@cjerad
Copy link
Contributor Author

cjerad commented Sep 3, 2021

Apparently GOPATH is not set in the CI environment. Is the preferred solution to add GOPATH to the CI environment (and update the check in toolchain.sh to avoid the "unbound variable" error) or just remove the check in toolchain.sh?

@bwagner5
Copy link
Contributor

bwagner5 commented Sep 3, 2021

Apparently GOPATH is not set in the CI environment. Is the preferred solution to add GOPATH to the CI environment (and update the check in toolchain.sh to avoid the "unbound variable" error) or just remove the check in toolchain.sh?

GOPATH is technically optional in go now since modules have taken over most of its use. We could change the check in toolchain.sh to check if $HOME/go/bin is in the path or if GOPATH is set and in the PATH. I'd guess the GHA has $HOME/go/bin in the PATH as part of the go install (famous last words).

@@ -11,6 +11,10 @@ tools() {
cd tools
go mod tidy
GO111MODULE=on cat tools.go | grep _ | awk -F'"' '{print $2}' | xargs -tI % go install %

if ! echo "$PATH" | grep -q "${GOPATH:-undefined}/bin\|$HOME/go/bin"; then
Copy link
Contributor

Choose a reason for hiding this comment

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

My path is actually at $HOME/workspaces/go. Thoughts on removing this check entirely and just printing a friendly message that isn't exact, but can at least provide users a hint.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing $HOME/workspaces/go is what your $GOPATH is set to? But if that weren't set for you, then go would default to $HOME/go

ellistarn
ellistarn previously approved these changes Sep 3, 2021
bwagner5
bwagner5 previously approved these changes Sep 3, 2021
Copy link
Contributor

@bwagner5 bwagner5 left a comment

Choose a reason for hiding this comment

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

🎉

@cjerad cjerad dismissed stale reviews from bwagner5 and ellistarn via 1f0e713 September 7, 2021 15:03
@cjerad cjerad force-pushed the cjerad-dev-guide-update branch from f731469 to 1f0e713 Compare September 7, 2021 15:03
Copy link
Contributor

@bwagner5 bwagner5 left a comment

Choose a reason for hiding this comment

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

lgtm

@bwagner5 bwagner5 merged commit eb5139e into main Sep 7, 2021
@bwagner5 bwagner5 deleted the cjerad-dev-guide-update branch September 7, 2021 16:34
gfcroft pushed a commit to gfcroft/karpenter-provider-aws that referenced this pull request Nov 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants