Skip to content

Conversation

@QuanMPhm
Copy link
Contributor

Closes #175. As the first step towards merging the account manager into our coldfront cloud plugin, the get_federated_user function in the Openshift allocator will now (through several functions) directly call the Openshift API. Much of the functions added are copied from the moc_openshift module in the account manager.

Aside from copying some functions,
implementation of this feature also involved:

  • A new resource attribute Identity Name for the Openshift idp
  • A new unit test for the get_federated_user function
  • Changes to the CI file to enable these new unit tests

@knikolla knikolla requested a review from larsks October 30, 2024 18:10
Copy link
Collaborator

@knikolla knikolla left a comment

Choose a reason for hiding this comment

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

Overall looks pretty good, some comments.

@QuanMPhm QuanMPhm requested a review from hakasapl November 15, 2024 19:37
@QuanMPhm QuanMPhm force-pushed the 175/merge_get_federated branch from 8839477 to 54922e7 Compare February 7, 2025 14:58
@QuanMPhm
Copy link
Contributor Author

QuanMPhm commented Feb 7, 2025

I have implemented all feedback so far. The devstack test broke because devstack release 2023.1 is no longer available. I assume this will be fixed with @knikolla 's upcoming PR

As the first step towards merging the account manager into our coldfront
cloud plugin, the `get_federated_user` function in the Openshift allocator
will now (through several functions) directly call the Openshift API.
Much of the functions added are copied from the `moc_openshift`
module in the account manager.

Aside from copying some functions,
implementation of this feature also involved:
- A new resource attribute `Identity Name` for the Openshift idp
- A new unit test for the `get_federated_user` function
- Changes to the CI file to enable these new unit tests
- A top-level logger for the Openshift allocator
- New additions to the package requirements
@QuanMPhm QuanMPhm force-pushed the 175/merge_get_federated branch from 54922e7 to 6752900 Compare February 7, 2025 15:09
@joachimweyl
Copy link

Fix merged, can we kick off the test again?

@QuanMPhm QuanMPhm requested a review from knikolla February 10, 2025 14:01
@QuanMPhm
Copy link
Contributor Author

@joachimweyl Yep, now the tests are passing

@knikolla
Copy link
Collaborator

I'll review this week, though don't want to rush this in for tomorrow's maintenance window.

Copy link
Collaborator

@knikolla knikolla left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Would like @larsks or @naved001 to do a review too.

After this one merges, can you please do quota operations, and then project operations?

Quota is higher priority than the users because currently that is the only one that requires changes to acct-mgt everytime we add a new quota type.

@QuanMPhm
Copy link
Contributor Author

Merge waiting on @larsks to resolve his comment.

@QuanMPhm QuanMPhm merged commit 2ad835f into nerc-project:main Mar 25, 2025
3 checks passed
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.

Allow direct communication to Openshift through get_federated_user function

5 participants