-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Metadata backend and frontend #19
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.
Awesome! We are addressing two major things with this PR.
Along with some minor comments I added we should also include
- New screenshots with the PR
- Metadata updates in the README
- Guidance around how you invoke tests for metadata with your test classes.
|
||
COPY requirements.txt . | ||
RUN pip3 install -r requirements.txt --target "${LAMBDA_TASK_ROOT}" | ||
ADD backend backend |
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 am curious if this change is intentional.
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.
Yes, this makes it so that we don't have to run the pip install step every time a file changes in the backend folder. Speeds up deploys a lot. If requirements.txt changes, the pip install will run.
|
||
return build_response(200, json.dumps({ "status": "OK" })) | ||
except ValidationError as ex: | ||
logger.info(traceback.format_exc()) |
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.
logger.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.
It's a client input validation error opposed to an application level error. Might be good for us to define how we use different log levels. I've often used conventions like these:
- debug: chatty details that are helpful to understanding during development
- info: any messages relating to nominal operation of the application, like a business transaction occurred, client validation failure
- warn: something that an operator may want to know about for tuning purposes... like, free memory or disk volume usage falling below threshold for safe operation.
- error: business transaction failure due to server-side faults, if too many of these happen, alert operators
- fatal: this application is about to crash, wake somebody up if necessary :)
Maybe the ValidationError
class could have a different name to increase some clarity.
response = { "status": "OK", "message": "{assetId} deleted".format(assetId=assetId) } | ||
return build_response(200, json.dumps(response)) | ||
except ValidationError as ex: | ||
logger.info(traceback.format_exc()) |
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.
logger.error here too
}, | ||
}); | ||
ddbtable.grantReadWriteData(fun); | ||
// fun.addToRolePolicy( |
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.
We can get rid of these comments.
|
||
class TableRow { | ||
idx!: number; | ||
name: string | null | undefined; |
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.
Why are these both null || undefined?
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.
The name/description fields are empty when new rows are added by the user.
self.test_process_output = open("/tmp/output.log", "wb") | ||
self.test_process = Popen([ | ||
"docker", "run", | ||
"-e", "METADATA_STORAGE_TABLE_NAME=vams-dev-us-east-1-MetadataStorageTable8114119D-1LY0MXNQ5XERU", |
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 should either be passed as a param.
loadingText="Loading..." | ||
submitEdit={async (blah, column, newValue) => { | ||
const item: TableRow = blah as TableRow; | ||
console.log("submitEdit", item, column, newValue); |
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.
can be removed
|
||
get(assetId) | ||
.catch((x) => { | ||
console.log("catch", x.response.status); |
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.
can be removed
} | ||
|
||
|
||
region = os.environ['AWS_REGION'] |
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 we may not need this, in lambda boto3 automatically picks up the right region
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.