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

Allow openstack esi node list to filter based on resource_class, owner, and lessee #155

Closed
wants to merge 1 commit into from

Conversation

skanthed
Copy link
Collaborator

No description provided.

@skanthed skanthed requested a review from tzumainn May 14, 2024 13:32
esi_leap/api/controllers/v1/node.py Outdated Show resolved Hide resolved
esi_leap/api/controllers/v1/node.py Outdated Show resolved Hide resolved
esi_leap/api/controllers/v1/node.py Show resolved Hide resolved
@skanthed skanthed force-pushed the apply-filter branch 2 times, most recently from 23a3db9 to c2ac65a Compare May 15, 2024 19:26
@skanthed
Copy link
Collaborator Author

@tzumainn updated the code with the changes.

@skanthed skanthed requested a review from tzumainn May 15, 2024 19:45
Copy link
Contributor

@tzumainn tzumainn left a comment

Choose a reason for hiding this comment

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

It's close, but missing a little!

Are you writing a python script to test locally? A script that creates an ironic client and passes things in?

esi_leap/api/controllers/v1/node.py Show resolved Hide resolved
@skanthed skanthed requested a review from tzumainn May 15, 2024 21:51
@skanthed
Copy link
Collaborator Author

@tzumainn I attempted to address the issue locally after installing the ironic-client, but encountered some problems. I will try again if necessary.

@skanthed
Copy link
Collaborator Author

@tzumainn I tested the script with ironic-client it's working fine.

Copy link
Contributor

@tzumainn tzumainn left a comment

Choose a reason for hiding this comment

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

One comment in-line. Can you also add unit tests? Thanks!

esi_leap/api/controllers/v1/node.py Outdated Show resolved Hide resolved
@skanthed
Copy link
Collaborator Author

@tzumainn Once the code looks good, I will add the unit-test.

@skanthed skanthed requested a review from tzumainn May 20, 2024 16:07
@skanthed skanthed force-pushed the apply-filter branch 4 times, most recently from 0d5f7e9 to 09debe6 Compare May 21, 2024 19:11
@skanthed skanthed self-assigned this May 21, 2024
@tzumainn
Copy link
Contributor

Can you rebase this commit to make it a single commit (and get rid of the merge commit)?


@mock.patch('esi_leap.common.ironic.get_node_list')
@mock.patch('esi_leap.common.keystone.get_project_list')
def test_get_resource_class_filter(self, mock_gpl, mock_gnl):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you rename this to test_get_all_resource_class_filter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure I will do this.

@tzumainn
Copy link
Contributor

tzumainn commented May 22, 2024 via email

@skanthed skanthed force-pushed the apply-filter branch 2 times, most recently from bb6480b to a9349f9 Compare May 22, 2024 15:05
@skanthed
Copy link
Collaborator Author

Due to merge conflict closing this and addressing it in #156

@skanthed skanthed closed this May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants