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

tink worker command line args #287

Merged
merged 4 commits into from
Oct 6, 2020

Conversation

displague
Copy link
Member

@displague displague commented Sep 9, 2020

Description

This PR makes most constants and environment variables for tink-worker configurable from the command line, environment variables, and falls back to a .tinkerbell.yaml (worker-id: 1234) (or .json) file.

$ ./tink-worker -w 212
Error: required flag(s) "docker-registry", "registry-password", "registry-username" not set
Usage:
  tink-worker [flags]

Flags:
  -r, --docker-registry string     Sets the Docker registry (DOCKER_REGISTRY)
  -h, --help                       help for tink-worker
  -s, --max-file-size int          Maximum file size in bytes (MAX_FILE_SIZE) (default 10485760)
  -m, --max-retry int              Maximum number of retries to attempt (MAX_RETRY) (default 3)
  -p, --registry-password string   Sets the registry-password (REGISTRY_PASSWORD)
  -u, --registry-username string   Sets the registry username (REGISTRY_USERNAME)
  -i, --retry-interval duration    Retry interval in seconds (RETRY_INTERVAL) (default 3ns)
  -v, --version                    version for tink-worker
  -w, --worker-id string           Sets the worker id (WORKER_ID)
  -l, --worker-log-level string    Sets the worker log level (panic, fatal, error, warn, info, debug, trace) (WORKER_LOG_LEVEL) (default "info")
  -t, --worker-timeout duration    Max duration to wait for worker to complete (WORKER_TIMEOUT)

Notably, TINKERBELL_CERT_URL lookups are checked from a client that is defined in a parent package. This variable has not been made configurable in the same way.

This is similar in spirit to #266

Considerable refactoring was done to clean up the use of global variables. The internal pkg contains a Worker and RegistryConnDetails struct which are context bags for the methods that needed these global variables. I imagine we will continue to evolve these types to find a better shape.

Why is this needed

When run from various environments, users may have an easier time manipulating either configuration files, command line arguments, or the environment. Also, by defining these variables with cobra/viper, users can leverage --help and we can eventually use this to generate man pages, shell auto-completions, and markdown docs.

Fixes: #

How Has This Been Tested?

How are existing users impacted? What migration steps/scripts do we need?

Known Bugs

  • --help does not include the environment variable hints which are shown when invalid args are supplied 🤔 (these hints are generated at runtime, we could just move them into the text description)

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

@gianarb gianarb added the needs-docs This label notifies the author of a PR that documentation is needed label Sep 10, 2020
@gianarb
Copy link
Contributor

gianarb commented Sep 10, 2020

That's a great effort @displague !! Now we have a page in our documentation for each service. You analyzed the topic well and you know the various ways to configure the tink-worker, do you mind adding a section in the tink-worker doc about this? https://tinkerbell.org/docs/services/tink/ Thanks

@displague
Copy link
Member Author

@gianarb That sounds like a good idea. I'll need to become a little more familiar with tink-worker before I can do that. I need to add tests to this pull request (and actually test it out myself 😅 ) before doing so.

cmd/tink-cli/cmd/root.go Outdated Show resolved Hide resolved
cmd/tink-worker/cmd/root.go Outdated Show resolved Hide resolved
cmd/tink-worker/cmd/root.go Outdated Show resolved Hide resolved
cmd/tink-worker/cmd/root.go Outdated Show resolved Hide resolved
cmd/tink-worker/cmd/root.go Outdated Show resolved Hide resolved
cmd/tink-worker/internal/registry.go Show resolved Hide resolved
cmd/tink-cli/cmd/root.go Outdated Show resolved Hide resolved
cmd/tink-worker/cmd/root.go Outdated Show resolved Hide resolved
cmd/tink-worker/cmd/root.go Outdated Show resolved Hide resolved
cmd/tink-worker/internal/action.go Outdated Show resolved Hide resolved
cmd/tink-worker/internal/action.go Outdated Show resolved Hide resolved
cmd/tink-worker/internal/registry.go Outdated Show resolved Hide resolved
cmd/tink-worker/internal/worker.go Outdated Show resolved Hide resolved
cmd/tink-worker/internal/worker.go Outdated Show resolved Hide resolved
cmd/tink-worker/main.go Outdated Show resolved Hide resolved
@displague displague force-pushed the tink-worker-opts branch 4 times, most recently from dda5ed9 to 3cf8bdb Compare September 16, 2020 04:23
@displague displague requested review from detiber and mmlb September 16, 2020 04:24
@displague
Copy link
Member Author

@mmlb @detiber I've addressed most of your concerns and commented on the ones that I didn't enact your recommendations.

Do either of you have an environment where you can connect this worker to the service and interact with it as you normally would?

cmd/tink-worker/cmd/root.go Outdated Show resolved Hide resolved
cmd/tink-worker/cmd/root.go Outdated Show resolved Hide resolved
cmd/tink-worker/cmd/root.go Outdated Show resolved Hide resolved
cmd/tink-worker/cmd/root.go Show resolved Hide resolved
cmd/tink-worker/cmd/root.go Outdated Show resolved Hide resolved
cmd/tink-worker/internal/worker.go Outdated Show resolved Hide resolved
cmd/tink-worker/internal/worker.go Outdated Show resolved Hide resolved
cmd/tink-worker/internal/worker.go Show resolved Hide resolved
cmd/tink-worker/internal/worker.go Outdated Show resolved Hide resolved
cmd/tink-worker/internal/worker.go Outdated Show resolved Hide resolved
@displague displague force-pushed the tink-worker-opts branch 3 times, most recently from 385caf4 to 0643ce4 Compare September 23, 2020 04:11
@codecov
Copy link

codecov bot commented Sep 24, 2020

Codecov Report

Merging #287 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #287   +/-   ##
=======================================
  Coverage   22.67%   22.67%           
=======================================
  Files          15       15           
  Lines        1270     1270           
=======================================
  Hits          288      288           
  Misses        963      963           
  Partials       19       19           
Impacted Files Coverage Δ
http-server/http_handlers.go 6.68% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a686336...e78b852. Read the comment docs.

cmd/tink-worker/cmd/root.go Outdated Show resolved Hide resolved
cmd/tink-worker/cmd/root.go Outdated Show resolved Hide resolved
cmd/tink-worker/cmd/root.go Show resolved Hide resolved
cmd/tink-worker/cmd/root.go Outdated Show resolved Hide resolved
cmd/tink-worker/cmd/root.go Outdated Show resolved Hide resolved
cmd/tink-worker/cmd/root.go Show resolved Hide resolved
mmlb
mmlb previously approved these changes Sep 28, 2020
…history

Signed-off-by: Marques Johansson <marques@packet.com>
Signed-off-by: Marques Johansson <marques@packet.com>
…ent) arguments

Signed-off-by: Marques Johansson <marques@packet.com>
Signed-off-by: Marques Johansson <marques@packet.com>
@displague displague requested review from detiber and gianarb October 5, 2020 13:09
Copy link
Contributor

@detiber detiber left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for all the hard work you put into this one @displague!

@displague displague added ready-to-merge Signal to Mergify to merge the PR. ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ and removed ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ labels Oct 6, 2020
@displague displague merged commit 1009dc3 into tinkerbell:master Oct 6, 2020
mergify bot added a commit to tinkerbell/osie that referenced this pull request Oct 6, 2020
…riable used for the worker ID (#147)

## Description

Adds support for the latest tink-worker changes to command line arguments/env variables after tinkerbell/tink#287 merged

## Why is this needed

Testing using the latest published images is broken, since tink-worker is looking for the ID env variable rather than WORKER_ID, this PR adds forward and backward compatible support.

## How Has This Been Tested?

Currently testing now using the tilt configuration I'm working on in https://github.com/detiber/tink/tree/kindDev, but it could also be tested using the vagrant or terraform setups with the changes needed to point to an osie build.

## How are existing users impacted? What migration steps/scripts do we need?

No additional migrations or scripts should be needed, it should not impact users.
@mmlb mmlb removed the ready-to-merge Signal to Mergify to merge the PR. label Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-docs This label notifies the author of a PR that documentation is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants