tink icon indicating copy to clipboard operation
tink copied to clipboard

Move to ff pkg for cli, move to logr for logging:

Open jacobweinstock opened this issue 3 years ago • 2 comments

Description

Viper and cobra are heavy and make this package more complicated than is needed. The ff package is lighter and has a lot less dependencies.

The logr package is more maintained and an easier to use logging package.

!! This PR breaks the backward compatibility of the CLI !!

CLI flags change

Before:

Tink Worker

Usage:
  tink-worker [flags]

Flags:
      --capture-action-logs        Capture action container output as part of worker logs (default true)
  -r, --docker-registry string     Sets the Docker registry (DOCKER_REGISTRY)
  -h, --help                       help for tink-worker
  -i, --id string                  Sets the worker id (ID)
      --max-file-size int          Maximum file size in bytes (MAX_FILE_SIZE) (default 10485760)
      --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)
      --retry-interval duration    Retry interval in seconds (RETRY_INTERVAL) (default 3s)
      --timeout duration           Max duration to wait for worker to complete (TIMEOUT) (default 1h0m0s)
  -v, --version                    version for tink-worker

After:

USAGE
  Run Tink Worker

FLAGS
  -id                   Worker ID.
  -log-level            Logging level. (default "info")
  -reg-pass             Container registry password.
  -reg-user             Container registry username.
  -registry             Container registry from which to pull images.
  -tink-cert-url        URL to Tink TLS certificate.
  -tink-grpc-authority  Hostname:port of Tink GRPC server.

Why is this needed

Fixes: #

How Has This Been Tested?

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

Checklist:

I have:

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

jacobweinstock avatar Apr 12 '22 04:04 jacobweinstock

Codecov Report

Merging #606 (4b22a05) into main (453f0fd) will decrease coverage by 0.39%. The diff coverage is 29.82%.

@@            Coverage Diff             @@
##             main     #606      +/-   ##
==========================================
- Coverage   46.22%   45.83%   -0.40%     
==========================================
  Files          56       57       +1     
  Lines        3310     3406      +96     
==========================================
+ Hits         1530     1561      +31     
- Misses       1690     1751      +61     
- Partials       90       94       +4     
Impacted Files Coverage Δ
cmd/tink-worker/worker/worker.go 0.00% <0.00%> (ø)
cmd/tink-worker/cmd/cmd.go 36.03% <36.03%> (ø)
cmd/tink-worker/worker/registry.go 80.76% <50.00%> (ø)
cmd/tink-worker/worker/container_manager.go 100.00% <100.00%> (+1.42%) :arrow_up:
cmd/tink-worker/worker/log_capturer.go 100.00% <100.00%> (ø)

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 453f0fd...4b22a05. Read the comment docs.

codecov[bot] avatar Apr 12 '22 04:04 codecov[bot]

Are we planning to make this a project wide decision for simplicity and ease of jumping between projects? I'm all for it.

chrisdoherty4 avatar Jul 30 '22 01:07 chrisdoherty4