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 developer documentation #1233

Closed
JochemTSR opened this issue Mar 25, 2024 · 3 comments
Closed

Update developer documentation #1233

JochemTSR opened this issue Mar 25, 2024 · 3 comments

Comments

@JochemTSR
Copy link

JochemTSR commented Mar 25, 2024

/kind feature

Describe the solution you'd like
While following the development guide, I ran into a few instances where the guide was inconsistent with the current development setup. While it is possible to figure out how to get started eventually, finding your bearings take longer than it has to and might dissuade future contributors from participating in development.

I'm trying to compile a list of areas where the documentation needs adjusting. Additions/corrections are more than welcome; I will keep the list updated. I can contribute a PR when all is good and ready.

Areas that need attention

  1. Currently the development guide states that make install-dev-prerequisites should be run to install the tools needed to start developing. This target does not exist in the Makefile (anymore); I believe the correct command is make all-tools. For some reason, the all-tools target is not listed in make help either.
  2. The guide states that users need to create and populate a tilt-settings.json file before starting Tilt. With the current Makefile this does not seem to be sufficient, as the tilt-up target fails if certain environment variables are not present. Instead, an .envrc file can be created from .envrc.sample; setting the variables in .envrc and sourcing it allows the make target to succeed. It seems like the .envrc is intended to be the source of truth for the settings, BUT the environment variables are overridden in the Tiltfile by default. The most obvious solution is to create a tilt-settings.json with an empty kustomize_substitutions key so the environment variables are not overridden. Is this the intended way of doing things?
  3. Documentation states that Tilt will be accessible from port 10350, but Tilt seems to be listening on port 10351.
  4. By default, Github selects the "Copy the main branch only" option when forking a branch. I learned the hard way that doing this will generate a lot of warnings and errors when using the Makefile, since it expects release tags to be present in the repository. This is not really a fault of the documentation, but probably worth mentioning if it's going to be updated.
  5. In order to run the E2E tests, docker buildkit needs to be enabled (e.g. export DOCKER_BUILDKIT=1. Without buildkit the BUILDPLATFORM is not populated and building will fail. Since buildkit is not the default (yet), I think this should be mentioned in the docs.
  6. hcloud is needed to perform E2E test, but is not included by default in make all-tools. This should probably be mentioned in the documentation (or hcloud could be installed through the makefile).
@kranurag7
Copy link
Contributor

Thank you very much for feedback and writing comprehensive issue.

  1. You're right and that needs to updated for sure. Regarding all-tools not being shown in make help output, we use an awk filtering there. We just add comment next to dependencies.
-all-tools: $(GOTESTSUM) $(go-cover-treemap) $(go-binsize-treemap) $(KIND) $(KUBECTL) $(CLUSTERCTL) $(CTLPTL) $(SETUP_ENVTEST) $(ENVSUBST) $(KUSTOMIZE) $(CONTROLLER_GEN) $(HELM)
+all-tools: $(GOTESTSUM) $(go-cover-treemap) $(go-binsize-treemap) $(KIND) $(KUBECTL) $(CLUSTERCTL) $(CTLPTL) $(SETUP_ENVTEST) $(ENVSUBST) $(KUSTOMIZE) $(CONTROLLER_GEN) $(HELM) ## Install Binaries in your ./hack/tools/bin directory
  1. We use .envrc in combination with direnv. Yes, I think the empty kustomize_substitutions should be good. Please note, in future (coming week) we are going to replace tilt-settings.json with tilt-settings.yaml So, that should be more readable for contributors.

  2. You're right, we can update the same. Tilt started on http://localhost:10351/

  3. Yes, that's a problem for now. GitHub shows us a checkbox in the UI. Copy the main branch only but checkbox is ticked by default. I think one can use the following to sync the tags.

git fetch upstream --tags
git push origin --tags
  1. I think this should be correct already. We use docker latest version when it's available in the package repository. After 23.0, BUILDKIT is enabled by default.

From Docker docs:
[BuildKit](https://github.com/moby/buildkit) is an improved backend to replace the legacy builder. BuildKit is the default builder for users on Docker Desktop, and Docker Engine as of version 23.0.

$ docker version
Client: Docker Engine - Community
 Version:           25.0.5
  1. Yes, you're right. I think we can add a make hcloud target for the same and add that as a dependency to the all-tools target.

This is where we require hcloud.

create_ssh_key() {
    echo "generating new ssh key"
    ssh-keygen -t ed25519 -f ${SSH_KEY_PATH} -N '' 2>/dev/null <<< y >/dev/null
    echo "importing ssh key "
    hcloud ssh-key create --name ${SSH_KEY_NAME} --public-key-from-file ${SSH_KEY_PATH}.pub
}

@kranurag7
Copy link
Contributor

@JochemTSR we updated the docs via this patch

If something is missing or something is still not working for you from the docs, then please let us know.
Thank you very much again for the descriptive issue. Really appreciate that.

@guettli
Copy link
Contributor

guettli commented Aug 15, 2024

@JochemTSR We have extended and beautified our docs: https://syself.com/docs/caph/getting-started/introduction

Please open a new issue, if something is missing or not easy to understand. Thank you

@guettli guettli closed this as completed Aug 15, 2024
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

No branches or pull requests

3 participants