feat: adds script for codeocean cleanup notification#50
Conversation
helen-m-lin
left a comment
helen-m-lin
left a comment
There was a problem hiding this comment.
Left some minor comments. Could you please link the related issue? I'm a little unclear about how this will integrate- who is going to create the csv file and exclude list?
helen-m-lin
left a comment
helen-m-lin
left a comment
There was a problem hiding this comment.
Changes to read from S3 look good. Added comments for unit test, but if this takes too much time to update, maybe those can be addressed in a separate ticket.
| self.example_job._is_s3_uri("/local/path/file.csv") | ||
| ) | ||
| self.assertFalse(self.example_job._is_s3_uri("file.csv")) | ||
| # Test _parse_s3_uri method |
There was a problem hiding this comment.
nit: each unit test should only test 1 method. Ideally split this up into test_is_s3_uri and test_parse_s3_uri
| with self.assertLogs(level="DEBUG") as captured: | ||
| exclude_items = self.example_job.read_exclude_list() | ||
| self.assertIsInstance(exclude_items, set) | ||
| self.assertIn("user2@example.com", exclude_items) |
There was a problem hiding this comment.
Should also assert the capsule. Maybe replace with self.assertEquals(["user2@example.com", "https://codeocean.com/capsule/12345"], exclude_items)
Similar comment for some of the other tests.
| csv_data = self.example_job.read_csv_file() | ||
| self.assertIsInstance(csv_data, list) | ||
| self.assertEqual(len(csv_data), 4) | ||
| for row in csv_data: |
There was a problem hiding this comment.
nit: This only checks the fieldnames. I'd suggest assertEqual the entire csv_data
|
Actually, before merging, can you run |
2 participants
Fixes https://github.com/AllenNeuralDynamics/aind-aws-infrastructure/issues/433