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

util.execute_command() real-time output and ocm.py verbosity level option #926

Merged
merged 8 commits into from
Sep 26, 2023

Conversation

manosnoam
Copy link
Contributor

@manosnoam manosnoam commented Sep 12, 2023

This PR includes two improvements for real-time verbosity in:

These changes are useful to see realtime log output of long running processes (for example when creating a cluster via OCM), as well as any shell command that being executed within ods-ci custom python functions.

@github-actions
Copy link
Contributor

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass %
359 0 0 359 100

@manosnoam manosnoam force-pushed the ocm_verbose branch 2 times, most recently from 7914e13 to ac5cc72 Compare September 12, 2023 20:34
@jstourac
Copy link
Member

Just wonder - I can see there are more occurrences of ocm (or /bin/ocm - would probably deserve to unify too?) command than just creating a cluster.

Shouldn't we be consistent and add this verbosity flag also on other occurrences too? I understand that adding this verbose flag doesn't make sense for some commands. But after very quick review e.g. delete_cluster, hibernate_cluster, resume_cluster, add_machine_pool, uninstall_addon, install_addon, create_idp? If we really don't want to do that, then we may want to update the help text for this new flag like this:

help="ocm logging verbosity level during OSD cluster creation",

@manosnoam
Copy link
Contributor Author

Just wonder - I can see there are more occurrences of ocm (or /bin/ocm - would probably deserve to unify too?)

Yes, not sure why it is done so, maybe for situations where a different version of ocm is required. We can fix it in another PR.

Shouldn't we be consistent and add this verbosity flag also on other occurrences too?

Sure, I added it to all relevant ocm commands that either creates, updates, or deletes resources, and skipped it for list/get commands.

Help says now:
ocm logging verbosity level for create/update/delete commands

@manosnoam
Copy link
Contributor Author

manosnoam commented Sep 14, 2023

An example for the ocm.py output when using ocm.py -v 2 :
image

An example when using ocm.py -v 1 :
image

And with ocm.py -v 0 :
image

@manosnoam manosnoam changed the title ocm.py verbosity level flag util.execute_command() real-time output and ocm.py verbosity level option Sep 14, 2023
ocm.py option to set verbosity level to the OCM logging using:
--ocm-verbose-level / -v <0-5>

The default verbosity level is set to 0.
A recommended level is 2. See more info at:
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/logging.md#what-method-to-use

Signed-off-by: manosnoam <[email protected]>
This is required, since main cli options (-o and -v) cannot be processed
after sub commands, as they will be treated as args for the sub commands.

It also fixes the `ocm.py --help` that will now display the main commands
at the top, before listing the sub commands:

```
optional arguments:
  -h, --help            show this help message and exit
  -o OCM_CLI_BINARY_URL, --ocmclibinaryurl OCM_CLI_BINARY_URL
                        ocm cli binary url (default: https://github.com/
                        openshift-online/ocm-cli/releases/download/v0.1.55/ocm-linux-amd64)
  -v OCM_VERBOSE_LEVEL, --ocm-verbose-level OCM_VERBOSE_LEVEL
                        ocm logging verbosity level (default: 0)

Available sub commands:
  {update_ocm_policy,update_ocm_channel,get_latest_osd_candidate_json,
  ocm_login,create_cluster,delete_cluster,hibernate_cluster,resume_cluster,
  delete_idp,get_osd_cluster_info,update_osd_cluster_info,install_rhods_addon,
  install_gpu_addon,add_machine_pool,uninstall_rhods_addon,
  install_rhoam_addon,uninstall_rhoam_addon,create_idp}
```

Signed-off-by: manosnoam <[email protected]>
@manosnoam manosnoam added the verified This PR has been tested with Jenkins label Sep 15, 2023
@manosnoam
Copy link
Contributor Author

Verified:
image

@bdattoma
Copy link
Contributor

did you try creating a cluster with the code from this PR?

@manosnoam
Copy link
Contributor Author

did you try creating a cluster with the code from this PR?

Yes I did, see:
#926 (comment)

@manosnoam manosnoam added the enhancements Bugfixes, enhancements, refactoring, ... in tests or libraries (PR will be listed in release-notes) label Sep 19, 2023
@bdattoma
Copy link
Contributor

did you try creating a cluster with the code from this PR?

Yes I did, see: #926 (comment)

right, sorry I missed that.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@aloganat
Copy link
Contributor

Changes looks good to me. We can merge it, if we have tested the workflows.

@manosnoam manosnoam merged commit 8438b2f into red-hat-data-services:master Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancements Bugfixes, enhancements, refactoring, ... in tests or libraries (PR will be listed in release-notes) verified This PR has been tested with Jenkins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants