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

Fixes #37053 - Add CV/LCE back host list params #922

Merged
merged 1 commit into from
Jan 22, 2024

Conversation

chris1984
Copy link
Member

@chris1984 chris1984 commented Jan 15, 2024

What are the changes introduced in this pull request?

  • With the new content_view_environments collection we were no longer pulling the data correctly; this has been updated.

Considerations taken when implementing this change?

  • This might not work with multiple cv/lce's since hammer list does not support using collection only the info command does. If this does not work with multiple cve's then when we get to that point, we will have to raise a redmine to hammer-cli core to add support to use collection with the list command.

What are the testing steps for this pull request?

  • Check out PR
  • Register a host to your Katello devel/production box
  • run hammer host list and see if the host Content view and Lifecycle environment fields are back

Before PR:

---|-------------------|------------------|------------|----------------|-------------------|--------------
ID | NAME              | OPERATING SYSTEM | HOST GROUP | IP             | MAC               | GLOBAL STATUS
---|-------------------|------------------|------------|----------------|-------------------|--------------
12 | devel.croberts.io | CentOS_Stream 8  |            | 192.168.41.128 | 00:0c:29:82:76:c6 | Warning
---|-------------------|------------------|------------|----------------|-------------------|--------------

With PR:

---|-------------------|------------------|------------|----------------|-------------------|---------------|---------------------------|----------------------
ID | NAME              | OPERATING SYSTEM | HOST GROUP | IP             | MAC               | GLOBAL STATUS | CONTENT VIEW              | LIFECYCLE ENVIRONMENT
---|-------------------|------------------|------------|----------------|-------------------|---------------|---------------------------|----------------------
12 | devel.croberts.io | CentOS_Stream 8  |            | 192.168.41.128 | 00:0c:29:82:76:c6 | Warning       | Default Organization View | Library
---|-------------------|------------------|------------|----------------|-------------------|---------------|---------------------------|----------------------

Copy link
Contributor

@qcjames53 qcjames53 left a comment

Choose a reason for hiding this comment

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

I tested changes with a variety of hosts with different properties, this seems to work as advertised across the board. Ack'ing.

I didn't know lists do not support collections, but that makes sense. Since other elements in hammer do not show fields when they are one-to-many, we could end up omitting this field in the future. Another idea: perhaps we could display a "(multiple)" string if there is more than one CV or LE on a host?

@chris1984
Copy link
Member Author

I tested changes with a variety of hosts with different properties, this seems to work as advertised across the board. Ack'ing.

I didn't know lists do not support collections, but that makes sense. Since other elements in hammer do not show fields when they are one-to-many, we could end up omitting this field in the future. Another idea: perhaps we could display a "(multiple)" string if there is more than one CV or LE on a host?

That is a better approach we can take, so we don't have to rely on hammer-core and get blocked if they are busy. Will do that when we get back to doing the multi CV stuff. Thanks for the review!!

@chris1984 chris1984 merged commit 9a6ae05 into Katello:master Jan 22, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants