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

feat: Remove useless CLI options from list command #3456

Merged
merged 3 commits into from
Oct 4, 2024
Merged

feat: Remove useless CLI options from list command #3456

merged 3 commits into from
Oct 4, 2024

Conversation

jirihnidek
Copy link
Contributor

@jirihnidek jirihnidek commented Sep 17, 2024 •
edited
Loading

  • Card ID: CCT-734
  • Removed --available and --consumed CLI options and other options
    related to listing available and consumed subscriptions
  • Only --installed (default) and --matches remain
  • Modified man page
  • Modified bash completion scripts
  • Removed many unit tests and modified existing unit tests
    related to installed products. Unit tests consider only
    SCA mode
  • Removed show_autosubscribe_output() and usage of this
    method, because it does not make sense to use it in SCA mode
  • cli_command/list.py contained several definitions of lists
    not related to list command. These definitions were moved
    to right Python modules. One list was renamed, because it
    is used only for testing ATM.

@cnsnyder cnsnyder requested review from a team and DuckBoss and removed request for a team September 17, 2024 12:18
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 17, 2024 •
edited
Loading

Coverage

Coverage (computed on Fedora latest) •
FileStmtsMissCoverMissing
subscription_manager/cli_command
   environments.py1094261%86, 89, 95, 98, 102, 109, 111–112, 114, 116, 133, 140–144, 146–153, 155, 157–162, 172, 188–194, 196–198
   list.py250100% 
   owners.py372143%45, 47–48, 50–60, 65, 67–69, 71–73
   register.py1974577%119, 121, 164–166, 168–169, 172, 183, 187, 194–197, 201–202, 206–207, 218, 246, 250–252, 261, 265, 291, 307–308, 344, 365, 373, 388–390, 399, 430, 440–441, 448–449, 454–458
   repos.py118694%130–131, 187, 213–214, 220
TOTAL17670455574% 

Tests Skipped Failures Errors Time
2470 14 💤 0 ❌ 0 🔥 33.568s ⏱️

@jirihnidek jirihnidek force-pushed the jhnidek/remove_list_available_consumed branch 2 times, most recently from e432ea7 to 5240636 Compare September 17, 2024 13:12
@jirihnidek
Copy link
Contributor Author

Note: not sure about CLI option --installed, because it is useless to have it there. On the other side we could keep it there for backward compatibility. I personally would like to remove --installed CLI option, because we going to break backward compatibility anyway.

@ptoscano
Copy link
Contributor

Note: not sure about CLI option --installed, because it is useless to have it there. On the other side we could keep it there for backward compatibility.

Let's keep it for now, and in case remove it later on (which will be easy to do).

ptoscano
ptoscano requested changes Sep 30, 2024
View reviewed changes
Copy link
Contributor

@ptoscano ptoscano left a comment

Choose a reason for hiding this comment

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

Mostly LGTM; added a couple of notes.

Thanks!

src/subscription_manager/cli_command/register.py Show resolved Hide resolved
test/test_printing_utils.py Outdated Show resolved Hide resolved
DuckBoss
DuckBoss requested changes Oct 1, 2024
View reviewed changes
Copy link
Contributor

@DuckBoss DuckBoss left a comment

Choose a reason for hiding this comment

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

Mostly LGTM too, I agree with Pino's change requests
and I've added my own questions/comments.

etc-conf/subscription-manager.completion.sh Show resolved Hide resolved
man/subscription-manager.8 Outdated Show resolved Hide resolved
src/subscription_manager/cli_command/list.py Outdated Show resolved Hide resolved
@jirihnidek jirihnidek force-pushed the jhnidek/remove_list_available_consumed branch from 5240636 to 2529472 Compare October 1, 2024 14:18
@jirihnidek
feat: Remove useless CLI options from list command
08f4595 
* Card ID: CCT-734
* Removed --available and --consumed CLI options and other options
  related to listing available and consumed subscriptions
* Only --installed (default) and --matches remains
* Modified man page
* Modified bash completion scripts
* Removed many unit tests and modified existing unit tests
  related to installed products. Unit tests consider only
  SCA mode
@jirihnidek jirihnidek force-pushed the jhnidek/remove_list_available_consumed branch from 2529472 to 286c3b2 Compare October 1, 2024 14:32
@ptoscano
Copy link
Contributor

ptoscano commented Oct 3, 2024

thanks for the updates; my two notes are still valid even in the current version

@jirihnidek jirihnidek force-pushed the jhnidek/remove_list_available_consumed branch from 286c3b2 to bb2ae4b Compare October 4, 2024 12:44
@jirihnidek
fix: Removed show_autosubscribe_output() method
7de19cd 
* Card ID: CCT-734
* Removed show_autosubscribe_output() and usage of this
  method, because it does not make sense to use it in SCA mode
@jirihnidek
refactor: Moved some definition of lists from list.py
a7cdd11 
* Card ID: CCT-734
* cli_command/list.py contained several definitions of lists
  not related to list command. These definitions were moved
  to right Python modules. One list was renamed, because it
  is used only for testing ATM.
@jirihnidek jirihnidek force-pushed the jhnidek/remove_list_available_consumed branch from bb2ae4b to a7cdd11 Compare October 4, 2024 13:29
@jirihnidek
Copy link
Contributor Author

thanks for the updates; my two notes are still valid even in the current version

Sorry, I overlooked your notes ... it should be addressed now.

ptoscano
ptoscano approved these changes Oct 4, 2024
View reviewed changes
Copy link
Contributor

@ptoscano ptoscano left a comment

Choose a reason for hiding this comment

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

LGTM, thanks; not merging it in case @DuckBoss wants to take another look

DuckBoss
DuckBoss approved these changes Oct 4, 2024
View reviewed changes
Copy link
Contributor

@DuckBoss DuckBoss left a comment

Choose a reason for hiding this comment

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

LGTM now, thanks for the changes 👍

@DuckBoss DuckBoss merged commit 5e094e3 into main Oct 4, 2024
16 checks passed
@ptoscano ptoscano deleted the jhnidek/remove_list_available_consumed branch October 7, 2024 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DuckBoss DuckBoss DuckBoss approved these changes

@ptoscano ptoscano ptoscano approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jirihnidek @ptoscano @DuckBoss