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.

Sign up for GitHub

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

Jump to bottom

cli: install - move not ready library log messages behind -verbose flag #810

Merged
merged 7 commits into from
Oct 28, 2021
Merged

cli: install - move not ready library log messages behind -verbose flag #810

merged 7 commits into from
Oct 28, 2021

Conversation

david-yu
Copy link
Contributor

@david-yu david-yu commented Oct 28, 2021 •
edited
Loading

Changes proposed in this PR:

  • Adds verbose flag to only show not ready messages when verbose is set to true

Default behavior with no -verbose flag.

❯ consul-k8s install -preset=demo

==> Pre-Install Checks
No existing installations found.
 ✓ No previous persistent volume claims found
 ✓ No previous secrets found

==> Consul Installation Summary
    Installation name: consul
    Namespace: consul
    Overrides:
    connectInject:
      enabled: true
    global:
      name: consul
    server:
      bootstrapExpect: 1
      replicas: 1

    Proceed with installation? (y/N) y

==> Running Installation
 ✓ Downloaded charts
 --> creating 1 resource(s)
 --> creating 27 resource(s)
 --> beginning wait for 27 resources with timeout of 10m0s
 ✓ Consul installed into namespace "consul"

Behaviour when -verbose is set to true

~/projects/consul-k8s/cli david-yu-cli-not-ready* 18s
❯ consul-k8s install -preset=demo -verbose

==> Pre-Install Checks
No existing installations found.
 ✓ No previous persistent volume claims found
 ✓ No previous secrets found

==> Consul Installation Summary
    Installation name: consul
    Namespace: consul
    Overrides:
    connectInject:
      enabled: true
    global:
      name: consul
    server:
      bootstrapExpect: 1
      replicas: 1

    Proceed with installation? (y/N) y

==> Running Installation
 ✓ Downloaded charts
 --> creating 1 resource(s)
 --> creating 27 resource(s)
 --> beginning wait for 27 resources with timeout of 10m0s
 --> DaemonSet is not ready: consul/consul. 0 out of 2 expected pods are ready
 --> DaemonSet is not ready: consul/consul. 0 out of 2 expected pods are ready
 --> DaemonSet is not ready: consul/consul. 0 out of 2 expected pods are ready
 --> DaemonSet is not ready: consul/consul. 0 out of 2 expected pods are ready
 --> DaemonSet is not ready: consul/consul. 0 out of 2 expected pods are ready
 --> DaemonSet is not ready: consul/consul. 0 out of 2 expected pods are ready
 --> DaemonSet is not ready: consul/consul. 0 out of 2 expected pods are ready
 --> DaemonSet is not ready: consul/consul. 0 out of 2 expected pods are ready
 --> DaemonSet is not ready: consul/consul. 0 out of 2 expected pods are ready
 --> DaemonSet is not ready: consul/consul. 0 out of 2 expected pods are ready
 --> DaemonSet is not ready: consul/consul. 0 out of 2 expected pods are ready
 --> DaemonSet is not ready: consul/consul. 0 out of 2 expected pods are ready
 --> DaemonSet is not ready: consul/consul. 0 out of 2 expected pods are ready
 --> DaemonSet is not ready: consul/consul. 0 out of 2 expected pods are ready
 --> DaemonSet is not ready: consul/consul. 0 out of 2 expected pods are ready
 --> DaemonSet is not ready: consul/consul. 0 out of 2 expected pods are ready
 --> DaemonSet is not ready: consul/consul. 0 out of 2 expected pods are ready
 --> DaemonSet is not ready: consul/consul. 0 out of 2 expected pods are ready
 --> DaemonSet is not ready: consul/consul. 0 out of 2 expected pods are ready
 --> DaemonSet is not ready: consul/consul. 1 out of 2 expected pods are ready
 --> DaemonSet is not ready: consul/consul. 1 out of 2 expected pods are ready
 ✓ Consul installed into namespace "consul"

How I've tested this PR:

Tested locally with reduced log messages.

How I expect reviewers to test this PR:

Checklist:

  • Tests added
  • CHANGELOG entry added

    HashiCorp engineers only, community PRs should not add a changelog entry.
    Entries should use present tense (e.g. Add support for...)

@david-yu david-yu requested a review from ndhanushkodi October 28, 2021 07:04
@david-yu david-yu added the area/cli CLI label Oct 28, 2021
david-yu
david-yu commented Oct 28, 2021
View reviewed changes
cli/cmd/install/install.go Outdated Show resolved Hide resolved
@david-yu david-yu requested a review from t-eckert October 28, 2021 17:34
@david-yu david-yu changed the title cli: install - remove not ready library log messages cli: install - move not ready library log messages behind -verbose flag Oct 28, 2021
ndhanushkodi
ndhanushkodi approved these changes Oct 28, 2021
View reviewed changes
Copy link
Contributor

@ndhanushkodi ndhanushkodi left a comment

Choose a reason for hiding this comment

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

looks good, thanks david!!

David Yu and others added 4 commits October 28, 2021 11:42
t-eckert
t-eckert approved these changes Oct 28, 2021
View reviewed changes
Copy link
Contributor

@t-eckert t-eckert left a comment

Choose a reason for hiding this comment

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

Sweet!

cli/cmd/install/install.go
c.UI.Output(logMsg, terminal.WithLibraryStyle())
} else {
// When verbose is not enabled, output all logs except not ready messages for resources
if !strings.Contains(logMsg, "not ready") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely works for now. Might be worth revisiting in the future. I might look into refactoring this later to have a generalized logger that can have a verbose setting. This is totally good for now though.

Update cli/cmd/install/install.go
4e35183 
Co-authored-by: Thomas Eckert <teckert@hashicorp.com>
@david-yu david-yu merged commit 5eaf12a into main Oct 28, 2021
@david-yu david-yu deleted the david-yu-cli-not-ready branch October 28, 2021 20:14
ottaviohartman pushed a commit to ottaviohartman/consul-k8s that referenced this pull request Nov 3, 2021
@ndhanushkodi
Rename lifecycle-sidecar to consul-sidecar (hashicorp#810)
3505db3 
This commit deprecates helm value lifecycleSidecarContainer and replaces it with
consulSidecarContainer. It also replaces all uses of lifecycle sidecar
with consul sidecar, including container names, commands, and flags.

This is to enable future functionality within the consul-sidecar
container, since it may no longer be only responsible for managing
lifecycle.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t-eckert t-eckert t-eckert approved these changes

@ndhanushkodi ndhanushkodi ndhanushkodi approved these changes

Assignees
No one assigned
Labels
area/cli CLI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@david-yu @ndhanushkodi @t-eckert