Add methods to CAS Management #129

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

Merged

smlindauer merged 17 commits into sassoftware:master from SilvestriStefano:cas-management-update

Sep 6, 2022

Contributor

SilvestriStefano commented Aug 2, 2022 •

edited

Loading

Hello,
I noticed that some methods are missing from the CAS Management service. I am thinking of adding the following for starters

list/create/delete sessions
load or unload a table
save a table
change scope of table

with related tests as well:

list/create/delete sessions
load or unload a table
save a table
change scope of table

Any suggestion/feedback is highly appreciated.

Question/curiosity: is there a reason why f-strings are not used?

Signed-off-by: Stefano Silvestri ssilvestri@olab-studio.com

SilvestriStefano added 3 commits

August 2, 2022 09:58


          add method in cas_management: update_state_table

cc0a503


          fix dict update in update_state_table

8cc3d4e


          add create_session method

50742a6

Collaborator

smlindauer commented Aug 2, 2022

Hey @SilvestriStefano,

We do not use f-strings since we still support users back to Python 3.5 and f-strings were introduced in Python 3.6.

I'm opening up the merge request to the git actions now, so those should begin to run momentarily.

If this is a WIP, would it be better to merge your updates into a new branch?

Contributor Author

SilvestriStefano commented Aug 2, 2022 •

edited

Loading

Hello,
I put the WIP tag because I haven't finished writing all the methods. I should have made a Draft pr, my bad.
If you prefer we can merge to a different branch.


          add delete_session method

eb5ac5f

SilvestriStefano marked this pull request as draft

August 2, 2022 12:31

SilvestriStefano added 10 commits

August 2, 2022 16:40


          add promote_table method

3b31f8d


          add save_table method

07114d8


          fix typos and format code

2d026f9


          add list_sessions method

f10e848


          add tests for sessions

442dc88


          add del_table method

b691826


          update upload_file method to allow query params and fix file upload

397335a


          add test for upload_file

eebeeb9


          add test for save_ and del_table

40ad154


          add test for promote_table

2f4ef6e

SilvestriStefano marked this pull request as ready for review

September 1, 2022 14:18

Contributor Author

SilvestriStefano commented Sep 1, 2022 •

edited

Loading

Hi,
I have added all the functionality that I set out to do. I had to modify the upload_table() method because it was not working for me.

Originally, in the post request was written files={name: file} which I changed to files={'files':(name,file)}. I followed the instructions from the request documentation. I have also added an extra optional keyword argument which allows for more details to be passed in the body of the request.

I have not added a test method for the update_state_table() method as it is used and tested in other tests. Also I do not have permissions to promote or change state on tables in the Samples caslib. This means I would need to upload the table first in some other caslib and then change the state, which is exactly what I do in the test_upload_file() method.

SilvestriStefano changed the title ~~[WIP] - Add methods to CAS Management~~ Add methods to CAS Management

Collaborator

smlindauer commented Sep 1, 2022

Hey @SilvestriStefano,

This looks great! I am in the process of reformatting the sasctl code base to fully follow PEP8 guidelines, so I made a number of comments in my review focused on those reformatting instances. The only logic change I suggested was with regards to adding a new function to handle the many key checks.

Thank you for creating the integration tests and their associated cassettes. I also noticed that cas_management is not subject to any unit tests, so I will add that to my to do list.

smlindauer requested changes

View reviewed changes

Collaborator

smlindauer left a comment

Initial review

src/sasctl/_services/cas_management.py

		@@ -8,6 +8,10 @@

		from .service import Service


		from typing import Union, TextIO

Collaborator

smlindauer Sep 1, 2022

Can we shift this back up just below "import os" to minimize extra whitespace in the imports? And to clearly separate python built-in, external library, and sasctl-specific imports.

Contributor Author

SilvestriStefano Sep 2, 2022

done.

src/sasctl/_services/cas_management.py Outdated

+                      """
+                      Returns a collection of sessions available on the CAS server.
+                      Params

Collaborator

smlindauer Sep 1, 2022

[Format Matching] Please use "Parameters" to match sasctl docstring format

Contributor Author

SilvestriStefano Sep 2, 2022

my bad. It should be fixed now.

src/sasctl/_services/cas_management.py Outdated

@@ @@ -22,7 +26,125 @@ class CASManagement(Service): @@
                   list_servers, get_server, _, _ = Service._crud_funcs("/servers", "server")
                   @classmethod
-                  def list_caslibs(cls, server, filter_=None):
+                  def list_sessions(cls, qparams: dict = None, server: str = None):

Collaborator

smlindauer Sep 1, 2022

change to query_params: more verbose variable names help with cognitive complexity and underscore instead of camelcase follows PEP8 S117 rule for variable naming convention

Contributor Author

SilvestriStefano Sep 2, 2022

done.

src/sasctl/_services/cas_management.py Outdated

+                      Params
+                      ------
+                      queryParams : dict, optional

Collaborator

smlindauer Sep 1, 2022

query_params

Contributor Author

SilvestriStefano Sep 2, 2022

done

src/sasctl/_services/cas_management.py Outdated

+                      """
+                      server = server or DEFAULT_SERVER
+                      if qparams is not None:

Collaborator

smlindauer Sep 1, 2022

query_params

Contributor Author

SilvestriStefano Sep 2, 2022

done

src/sasctl/_services/cas_management.py Outdated

+                          if ("sourceTableName" not in qParam.keys()
+                          and "quiet" not in qParam.keys()
+                          and "removeAcs" not in qParam.keys()):
+                              raise Exception(

Collaborator

smlindauer Sep 1, 2022

Can we use ValueError here instead of a general exception?

Contributor Author

SilvestriStefano Sep 2, 2022 •

edited

Loading

done in the second helper function check_required_key

src/sasctl/_services/cas_management.py Outdated

+                                  "Missing required query parameters `sourceTableName`, `quiet`, `removeAcs`"
+                              )
+                          else:
+                              query = qParam

Collaborator

smlindauer Sep 1, 2022

query_params

Contributor Author

SilvestriStefano Sep 2, 2022

done

src/sasctl/_services/cas_management.py Outdated

+                      else:
+                          raise ValueError(
+                              "The only acceptable query parameters are %s and must be passed in a dictionary"
+                              % (allowedQ)

Collaborator

smlindauer Sep 1, 2022

allowed_queries & unnecessary parentheses

Contributor Author

SilvestriStefano Sep 2, 2022

done

src/sasctl/_services/cas_management.py Outdated

+                      server : str, optional
+                          Server where the `caslib` is registered.
+                          Defaults to `cas-shared-default`.
+                      qparams: dict, optional

Collaborator

smlindauer Sep 1, 2022

query_params

Contributor Author

SilvestriStefano Sep 2, 2022

done

src/sasctl/_services/cas_management.py Outdated

+              from typing import Union, TextIO
               DEFAULT_SERVER = "cas-shared-default"
               DEFAULT_CASLIB = "casuser"

Collaborator

smlindauer Sep 1, 2022

Can we add a classless function up here to handle the many key checks? Something like:

check_valid_keys(valid_keys, input_keys, parameter_name):
if not all(key in valid_keys for key in input_keys):
raise ValueError("The only acceptable values for %s are %s" % (parameter_name, valid_keys))

i.e. change

elif (
format_ == 'csv' and
not all(key in allowed_body_csv for key in input_keys)
):
raise ValueError(
"The parameters specific to a csv file are %s." % allowed_body_csv
)

to

elif format_ == "csv":
check_valid_keys(allowed_body_csv, input_keys, "csv parameters")

Contributor Author

SilvestriStefano Sep 2, 2022 •

edited

Loading

As requested, I have added two functions that handles the many checks: one for checking that the keys entered are valid, and one that checks that required keys are being passed as inputs.

Contributor Author

SilvestriStefano commented Sep 1, 2022

Hello @smlindauer,
I ll make all the suggested changes.

Thank you for reviewing.

SilvestriStefano added 2 commits

September 2, 2022 10:06


          make review suggested changes and add extra check in test_del_table

351dbad


          make review suggested changes missed in the previous commit

f00d94e

Contributor Author

SilvestriStefano commented Sep 2, 2022

I want to mention that I changed the DEFAULT_CASLIB value to Samples since casuser is not a valid caslib. The caslib associated to a user is CASUSER(user_id) were user_id is the id of the user (for example in my integration tests I have used CASUSER(X1064007) )

Collaborator

smlindauer commented Sep 2, 2022

I want to mention that I changed the DEFAULT_CASLIB value to Samples since casuser is not a valid caslib. The caslib associated to a user is CASUSER(user_id) were user_id is the id of the user (for example in my integration tests I have used CASUSER(X1064007) )

Should we go with Samples or would Public make more sense? I believe Public and Samples are always created, but Public is more likely to contain CAS data that users would be using.

smlindauer approved these changes

View reviewed changes


          change default caslib to Public

e245406

Contributor Author

SilvestriStefano commented Sep 5, 2022

@smlindauer you are right. The "Public" caslib makes more sense as the default one. I made the change.

smlindauer merged commit a242abf into sassoftware:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet