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

Update crc_squeue #231

Closed
wants to merge 52 commits into from
Closed

Update crc_squeue #231

wants to merge 52 commits into from

Conversation

chnixi
Copy link
Contributor

@chnixi chnixi commented May 7, 2024

Added warning message to notify users within 30 days of proposal expiration date.

@chnixi chnixi requested a review from Comeani May 7, 2024 01:45
Comeani and others added 7 commits May 7, 2024 15:20
Use Slurm utility function to check account
Import correct modules
* Update crc_proposal_end.py

Added authentication procedure

* Update crc_proposal_end.py

* Update crc_proposal_end.py

* Update crc_proposal_end.py

Updated requests with group_id

* Update crc_proposal_end.py

Update requests

* working version of crc_proposal_end

* remove unused imports

---------

Co-authored-by: Nickolas Comeau <[email protected]>
Copy link
Contributor

@Comeani Comeani left a comment

Choose a reason for hiding this comment

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

@chnixi Looking through this, I realized the arguments do not take in an account or set a default account to use as args.account. If you can add that and test with a example AllocationRequest under the sam group or something (you can set it up in the admin console for keystone from Viz) just so we know the functionality works, I think the changes here are OK to include. Otherwise we can try to add it as functionality separate from merging and tagging a release downstream of #233.

Base automatically changed from keystone_update to main May 8, 2024 20:12
@Comeani
Copy link
Contributor

Comeani commented May 17, 2024

@chnixi There were some issues with the imports and some syntax errors that I fixed.

The utility functions for touching keystone have been changed a bit since this last had main merged in. They should still return the right things, I think I just changed the names to be more accurate. Can you make the corresponding changes here?

Updated keystone utility function name to get active requests
@chnixi
Copy link
Contributor Author

chnixi commented May 22, 2024

Not sure why it is giving the error "AttributeError: 'function' object has no attribute 'getuser". Did we change something in getpass? crc-scancel uses the same routine to get the user name.

@Comeani
Copy link
Contributor

Comeani commented May 22, 2024

from getpass import getpass

This is not importing getpass the module, but getpass the function inside the getpass module.

You can change the import to from getpass import getpass, getuser and the change line 47 to user = f'-u {getuser()}'

Import entire getpass module
@chnixi
Copy link
Contributor Author

chnixi commented May 22, 2024

Fixed this, Now it is complaining about: 'username': os.environ["USER"],
File "/usr/lib64/python3.8/os.py", line 675, in getitem
raise KeyError(key) from None
KeyError: 'USER'

The call where this happens is the same as in crc-usage:
auth_header = get_auth_header(KEYSTONE_URL,
{'username': os.environ["USER"],
'password': getpass("Please enter your CRC login password:\n")})

@Comeani
Copy link
Contributor

Comeani commented May 22, 2024

Is the $USER environment variable defined in the shell you are running from? I'll see if I can reproduce the problem

@Comeani
Copy link
Contributor

Comeani commented May 22, 2024

os.environ does not give me any issues running on the cluster, I fixed the imports so they work properly now.

I see where you ran into that problem. It looks like the tests break in the CI when trying to find the USER environment variable. I'll see if I can find a fix for this.

@Comeani
Copy link
Contributor

Comeani commented May 22, 2024

Adding the call to keystone makes it impossible to run some of these tests properly because the CI can't get the password for authenticating, and even if it could it wouldn't be able to reach out deployment of keystone.

I think we make an issue to rework utils so it can do it's functionality independent from the application logic for any given wrapper (Daniel gave me some note on how I might start doing this) and be more easily mocked in these tests.

If we can test that the output for a expiring allocation shows up for this version of crc-squeue on the cluster, I'm content to merge it in and revisit the tests another time (or just defer until we have email notifications in place)

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.

3 participants