-
Notifications
You must be signed in to change notification settings - Fork 0
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
✨ 454 - implement ega job flow #460
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is functional and no major problems so I approved with bits of code I would fix. In particular, we suppress the axios error from that one request and don't log what happened.
case 429: | ||
throw new TooManyRequestsError(error.message); | ||
default: | ||
throw new ServerError('Unexpected Axios Error'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you aren't throwing or logging any of the information from the axios error!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be adding more error mgmt/retries in #455
import { getUsers } from './services/users'; | ||
import { isSuccess } from './types/results'; | ||
import { getApprovedUsers } from './utils'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our function naming is unclear for reader. For the two functions getUsers
and getApprovedUsers
, one is fetching from the DACO DB, and the other from the EGA API. There is no indication of which is which.
src/jobs/ega/services/permissions.ts
Outdated
export const processPermissionsForApprovedUsers = async ( | ||
egaClient: EgaClient, | ||
egaUsers: EgaDacoUserMap, | ||
datasets: Dataset[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dataset
is super generic type name, not sure what this contains... I looked up the type and its just an id, name, and description... still not sure.
Perhaps just prefixing Ega
to it is enough to indicate its a structure from their API? EgaDataset
?
src/jobs/ega/services/permissions.ts
Outdated
if (existingPermission.data.success.length === 0) { | ||
// create permission request, add to requestList | ||
const permissionRequest = createPermissionRequest( | ||
approvedUser.username, | ||
dataset.accession_id, | ||
); | ||
permissionRequests.push(permissionRequest); | ||
} | ||
} else { | ||
logger.info(`Error fetching existing permission: ${existingPermission.message}`); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Success length of 0 means create request, but success length of 1 or greater means error fetching?
I feel like i don't understand whats happening here but potentially this logic is reversed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's not written very clearly - the else
is actually handling the catch
clause from getPermissionByDatasetAndUserId
, which returns a failure('SERVER_ERROR')
. I'll rewrite as a switch statement so it's easier to read.
The GET /permissions
endpoints always return an array, so an empty list means the approved user has no permissions for the requested dataset.
export const processPermissionsForDataset = async ( | ||
client: EgaClient, | ||
datasetAccessionId: DatasetAccessionId, | ||
approvedUsers: EgaDacoUserMap, | ||
): Promise<void> => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just logs outcomes? No data returned to compile a report?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add reporting in #455
src/jobs/ega/services/permissions.ts
Outdated
const setSize = permissionsSet.size; | ||
if (setSize > 0) { | ||
logger.debug(`There are ${permissionsSet.size} permissions to remove.`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy variable, use once, then use original variable...
src/jobs/ega/types/responses.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Food for thought: I would consider being less strict with all the response validations, only check for data that you need to use. For example, I don't think we ever use EgaPermissionRequest.full_name
or EgaPermissionRequest.email
, so we should be fine accepting a response without that property. This will prevent our script from failing if they change part of their API that we aren't using.
#461) * add pThrottle source code dir, add throttling to all egaClient functions
Work for #454
Implements (mostly happy path) EGA permission reconciliation flow. Error handling needs to be improved in main job function and service calls - in most cases errors encountered should not stop the script from running, but would be useful to report/collect somehow, for debugging/possible retry scenarios.
Note: The logger setup is becoming a PITA, might be useful to replace with more robust logger that implements what
buildMessage
is doing automaticallyEgaClient
400 - Permission does not exist
torevokePermissions
- this was caught while debugging ega api pagination issueJob
/services
Utils
createPermissionApprovalRequest
func for creating permission approval request objectsTypes
nullable()
to some of the response type fields. These values are not used in the recon flowPERMISSION_DOES_NOT_EXIST
errors.ts
to types dir, adds custom errors forBadRequest
andServerError
Code cleanup/readability
services/
folder for service functions related to users and permissions need in the main reconciliation job