Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

fix(cli): fix uninstall cmd not showing smi info #4235

Merged

Conversation

johnsonshi
Copy link
Contributor

@johnsonshi johnsonshi commented Oct 8, 2021

The osm uninstall (osm delete) command prints out a list of
osm meshes before prompting the user for action. The column
for supported SMI versions in the uninstall prompt shows
"Unknown" because the localPort argument (required
for port-forwarding to obtain SMI info) is missing.

Resolves #4195

Signed-off-by: Johnson Shi [email protected]

Description:

Testing done:

Affected area:

Functional Area
New Functionality [ ]
CI System [ ]
CLI Tool [x]
Certificate Management [ ]
Control Plane [ ]
Demo [ ]
Documentation [ ]
Egress [ ]
Ingress [ ]
Install [ ]
Networking [ ]
Observability [ ]
Performance [ ]
SMI Policy [ ]
Security [ ]
Sidecar Injection [ ]
Tests [ ]
Upgrade [ ]
Other [ ]

Please answer the following questions with yes/no.

  1. Does this change contain code from or inspired by another project? N

    • Did you notify the maintainers and provide attribution?
  2. Is this a breaking change? N

@johnsonshi johnsonshi requested a review from a team as a code owner October 8, 2021 21:44
@johnsonshi johnsonshi force-pushed the osm-uninstall-missing-smi-info branch from 7fce016 to 9295f60 Compare October 8, 2021 21:44
@codecov-commenter
Copy link

codecov-commenter commented Oct 8, 2021

Codecov Report

Merging #4235 (9295f60) into main (75562ac) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4235      +/-   ##
==========================================
- Coverage   69.57%   69.57%   -0.01%     
==========================================
  Files         211      211              
  Lines       11436    11438       +2     
==========================================
+ Hits         7957     7958       +1     
- Misses       3430     3431       +1     
  Partials       49       49              
Flag Coverage Δ
unittests 69.57% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/cli/uninstall.go 79.24% <100.00%> (+0.81%) ⬆️
pkg/certificate/rotor/rotor.go 84.37% <0.00%> (-3.13%) ⬇️

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 75562ac...9295f60. Read the comment docs.

@johnsonshi
Copy link
Contributor Author

#4195

f.BoolVar(&uninstall.deleteNamespace, "delete-namespace", false, "Attempt to delete the namespace after control plane components are deleted")
f.Uint16VarP(&uninstall.localPort, "local-port", "p", constants.OSMHTTPServerPort, "Local port to use for port forwarding")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest adding "Default is 9091" to the description

@johnsonshi johnsonshi merged commit 22c1b4d into openservicemesh:main Oct 21, 2021
allenlsy pushed a commit to allenlsy/osm that referenced this pull request Dec 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI command "osm delete" reports "SMI SUPPORTED" Unknown
4 participants