fix(cli): skip version check on install by vadasambar · Pull Request #5046 · keptn/keptn

vadasambar · 2021-08-25T16:09:27Z

This PR

skips version check on keptn install

Related Issues

Fixes #3158

How to test

Run keptn install locally without a cluster running. You should not see a Warning message like this.
Run any other command like keptn auth or keptn uninstall without a cluster running and you should see a Warning message like this.

vadasambar · 2021-08-25T16:12:48Z

cli/cmd/root.go

PersistentPreRun cannot be skipped conditionally based on the subcomand.

codecov · 2021-08-26T06:03:03Z

Codecov Report

Merging #5046 (96a01e6) into master (d9ac0ed) will decrease coverage by 0.47%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master    #5046      +/-   ##
==========================================
- Coverage   52.19%   51.71%   -0.48%     
==========================================
  Files         298      292       -6     
  Lines       17568    17309     -259     
  Branches      690      690              
==========================================
- Hits         9170     8952     -218     
+ Misses       7536     7501      -35     
+ Partials      862      856       -6

Impacted Files	Coverage Δ
cli/cmd/root.go	`63.75% <92.85%> (+6.39%)`	⬆️
cli/cmd/version.go	`51.61% <0.00%> (-4.31%)`	⬇️
secret-service/pkg/repository/scopes.go
secret-service/pkg/backend/registry.go
secret-service/pkg/backend/secretbackend_k8s.go
secret-service/pkg/handler/secrethandler.go
secret-service/pkg/common/utils.go
secret-service/pkg/handler/common.go

Signed-off-by: Suraj Banakar (vadasambar) <[email protected]>

- `panic`'ing right now Signed-off-by: Suraj Banakar (vadasambar) <[email protected]>

johannes-b · 2021-09-02T15:04:36Z

Hello @vadasambar,
thanks for working on this issue. Can you please address the broken unit test: https://github.com/keptn/keptn/pull/5046/checks?check_run_id=3492104219?

Signed-off-by: Suraj Banakar (vadasambar) <[email protected]>

vadasambar · 2021-09-02T16:31:27Z

Hey @johannes-b, thank you for the comment. I have fixed the test.

vadasambar · 2021-09-02T16:34:11Z

cli/cmd/root.go

+func runVersionCheck(vChecker *version.VersionChecker, flags []string) {
+	// Server version won't be available during `install`
+	// because the Server is not installed yet
+	if isInstallSubCommand(flags) {


Doing this in runVersionCheck instead of PersistentRun because it is easier to test runVersionCheck (we already have code for doing this here)

vadasambar · 2021-09-02T16:35:44Z

cli/cmd/root.go

 	}
 }
+
+func isInstallSubCommand(flags []string) bool {


TODO: Use a different variable name here. flags here means flags like -q and subcommands like install in a slice e.g., []string{"-q", "install"}

vadasambar · 2021-09-02T16:36:24Z

cli/cmd/root_test.go

 		metadataStatus   int
 		metadataResponse keptnapimodels.Metadata
 		cliVersion       string
+		flags            []string


TODO: Use a different variable name here. flags here means flags like -q and subcommands like install in a slice e.g., []string{"-q", "install"}

vadasambar · 2021-09-02T16:38:54Z

cli/cmd/root_test.go

 			wantOutput:     "* Warning: could not check Keptn server version: received invalid response from Keptn API\n",
 		},
+		{
+			name:            "skip version check for 'keptn install'",


I am writing unit tests related keptn install in root_test.go because the code to skip version check on install subcommand is in root.go.

vadasambar · 2021-09-02T16:40:35Z

cli/cmd/root_test.go


 			out := r.revertStdErr()
-			if !strings.Contains(out, tt.wantOutput) {
+			if tt.wantOutput != "" && !strings.Contains(out, tt.wantOutput) {


Check if tt.wantOutput is empty here because some tests have wantOutput as empty. They use doNotWantOutput instead.

- because `flags` does not include subcommands Signed-off-by: Suraj Banakar (vadasambar) <[email protected]>

- exclude `keptn` from args Signed-off-by: Suraj Banakar (vadasambar) <[email protected]>

- this is an attempt to fix reviewdog complaining about `shellwords` not being used Signed-off-by: Suraj Banakar (vadasambar) <[email protected]>

sonarqubecloud · 2021-09-09T15:34:17Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

bacherfl

lgtm

vadasambar force-pushed the fix/3158/skip-version-check-for-install branch from 9385d56 to 8cea6b3 Compare August 25, 2021 16:09

vadasambar commented Aug 25, 2021

View reviewed changes

cli/cmd/root.go Outdated

Copy link
Copy Markdown

Member Author

vadasambar Aug 25, 2021

Choose a reason for hiding this comment

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

PersistentPreRun cannot be skipped conditionally based on the subcomand.

fix(cli): skip version check on install

1b076ed

Signed-off-by: Suraj Banakar (vadasambar) <[email protected]>

vadasambar force-pushed the fix/3158/skip-version-check-for-install branch from 8cea6b3 to 1b076ed Compare August 27, 2021 16:53

fix(cli): WIP add unit test for install

9b21670

- `panic`'ing right now Signed-off-by: Suraj Banakar (vadasambar) <[email protected]>

fix(cli): add working unit tests for install flag

e8d29ee

Signed-off-by: Suraj Banakar (vadasambar) <[email protected]>

vadasambar force-pushed the fix/3158/skip-version-check-for-install branch from f1da7a1 to e8d29ee Compare September 2, 2021 16:30

vadasambar commented Sep 2, 2021

View reviewed changes

fix(cli): rename flags -> args

a182d65

- because `flags` does not include subcommands Signed-off-by: Suraj Banakar (vadasambar) <[email protected]>

vadasambar marked this pull request as ready for review September 6, 2021 15:58

vadasambar requested a review from a team as a code owner September 6, 2021 15:58

vadasambar added 2 commits September 6, 2021 21:32

fix(cli): fix root test os args

a54addc

- exclude `keptn` from args Signed-off-by: Suraj Banakar (vadasambar) <[email protected]>

fic(cli): add explicit import for shellwords

96a01e6

- this is an attempt to fix reviewdog complaining about `shellwords` not being used Signed-off-by: Suraj Banakar (vadasambar) <[email protected]>

vadasambar force-pushed the fix/3158/skip-version-check-for-install branch from 8bc5b41 to 96a01e6 Compare September 9, 2021 15:32

vadasambar mentioned this pull request Sep 9, 2021

keptn set config AutomaticVersionCheck false doesn't work #5218

Closed

12 tasks

bacherfl approved these changes Sep 10, 2021

View reviewed changes

bacherfl merged commit a5aecd4 into keptn:master Sep 10, 2021

vadasambar mentioned this pull request Sep 30, 2021

REQUEST: Membership member for vadasambar keptn/community#56

Closed

7 tasks

Conversation

vadasambar commented Aug 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This PR

Related Issues

How to test

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

johannes-b commented Sep 2, 2021

Uh oh!

vadasambar commented Sep 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Sep 9, 2021

Uh oh!

bacherfl left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vadasambar commented Aug 25, 2021 •

edited

Loading

codecov bot commented Aug 26, 2021 •

edited

Loading