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

Fix for getting optional information about openshift cluster version. #137

Merged

Conversation

ratsuf
Copy link
Contributor

@ratsuf ratsuf commented May 7, 2021

Description

In the case of openshift distribution, there is an invocation of kubectl get clusterversion. But such a type of resource not present in OKD 3.11 and lower. As a result - #133
Because the result of 'clusterversion' used just for reference in the logs, I suggest the following fixes:

Fixes

  1. Added optional_invoke method because current invoke stops Cerberus execution with UnboundLocalError in case of exception.
  2. Used the optional invoke method for collecting reference information about openshift cluster.

@comet-perf-ci
Copy link

Can one of the admins verify this patch?

Copy link

@seanogor seanogor left a comment

Choose a reason for hiding this comment

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

LGTM

@amitsagtani97 amitsagtani97 added the ok to test Runs functional tests in CI label May 7, 2021
@amitsagtani97
Copy link
Contributor

/rerun all

@comet-perf-ci
Copy link

Test Result Duration
test_daemon_disabled Pass 00:00:29
test_detailed_data_inspection Pass 00:00:54
test_slack_integration Pass 00:01:06
Check Gold time (s) PR time (s) % Change
"check_master_taint" .180727 .187929 3.98
"entire_iteration" 3.587589 3.354181 -6.50
"sleep_tracker" 1.280952 1.192856 -6.87
"watch_cluster_operators" .522088 .518089 -.76
"watch_csrs" .128074 .128175 .07
"watch_namespaces" 1.312096 1.195382 -8.89
"watch_nodes" .252358 .242000 -4.10

@paigerube14
Copy link
Collaborator

The CI needs to be updated to be 120 characters not 110. Will be fixed in PR

LGTM

@chaitanyaenr chaitanyaenr added the Needs Rebase Needs to be rebased before merging label May 7, 2021
@chaitanyaenr
Copy link
Collaborator

@ratsuf we might want to rebase the PR.

@ratsuf ratsuf force-pushed the clusterversion_getting_issue_133 branch from 1672b36 to 0c67f0f Compare May 8, 2021 12:38
@ratsuf
Copy link
Contributor Author

ratsuf commented May 8, 2021

@chaitanyaenr I've rebased the PR.

@ratsuf ratsuf force-pushed the clusterversion_getting_issue_133 branch from 0c67f0f to 66d61f9 Compare May 8, 2021 12:55
@ratsuf
Copy link
Contributor Author

ratsuf commented May 11, 2021

@chaitanyaenr should I change something else?

@chaitanyaenr
Copy link
Collaborator

/rerun all

Copy link
Collaborator

@chaitanyaenr chaitanyaenr left a comment

Choose a reason for hiding this comment

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

LGTM

@comet-perf-ci
Copy link

Test Result Duration
test_daemon_disabled Pass 00:00:29
test_detailed_data_inspection Pass 00:00:51
test_slack_integration Pass 00:01:04
Check Gold time (s) PR time (s) % Change
"check_master_taint" .185639 .200576 8.04
"entire_iteration" 3.370921 3.446879 2.25
"sleep_tracker" 1.204920 1.221290 1.35
"watch_cluster_operators" .513252 .527975 2.86
"watch_csrs" .126635 .127082 .35
"watch_namespaces" 1.206616 1.226955 1.68
"watch_nodes" .232474 .228316 -1.78

@chaitanyaenr chaitanyaenr merged commit a77601b into krkn-chaos:master May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Rebase Needs to be rebased before merging ok to test Runs functional tests in CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants