-
Notifications
You must be signed in to change notification settings - Fork 7
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
Ingest adds keyhash to files db table #1073
base: main
Are you sure you want to change the base?
Conversation
ecec008
to
ceecb05
Compare
1cfac07
to
5fc3703
Compare
This PR should not have to be dependent on #1036 |
9c42595
to
f6ab0a7
Compare
ca919ba
to
910a2db
Compare
cba4dff
to
97893e3
Compare
910a2db
to
b6b5fcd
Compare
440b590
to
b488b73
Compare
85f4ef9
to
0f68dbe
Compare
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.
Very good job. Just an extremely minor thing, in the PR's description I would change the part saying that it prints a warning if the keyhash does not exist in the sda.encryption_keys
table because in reality it will through an error. I know that maybe is too much but my eye caught it :)
Thanks @kostas-kou! The description was indeed outdated, very good that you noticed! It's updated now. |
exit 1 | ||
fi | ||
|
||
apt-get -o DPkg::Lock::Timeout=60 update >/dev/null |
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 is inefficient and waste of time to (potentially) run apt-get update
multiple times. It can be refactored to run apt-get update
only once.
echo "ingestion and verification test completed successfully" | ||
# check that the files have key hashes assigned | ||
key_hashes="$(psql -U postgres -h postgres -d sda -At -c "select distinct key_hash from sda.files" | wc -l)" | ||
if [ "$key_hashes" -lt 0 ]; then |
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.
if [ "$key_hashes" -lt 0 ]; then | |
if [ "$key_hashes" -eq 0 ]; then |
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 result of wc -l
can never be negative.
err = db.SetKeyHash(keyhash, fileID) | ||
if err != nil { | ||
log.Errorf("Key hash %s could not be set for fileID %s: (%s)", keyhash, fileID, err.Error()) | ||
if err = delivered.Nack(false, true); err != nil { |
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 would use a different variable name for the inner err
to increase the clarity. However, maybe this not inline with the style of sda
? What do others think?
|
||
return err | ||
} | ||
if rowsAffected, _ := result.RowsAffected(); rowsAffected == 0 { |
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 ignore the potentially returned error 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.
Looks good. I added a few comments in the integrations test shell scripts and some questions regarding error handling in the go code.
Related issue(s) and PR(s)
This PR closes #893.
It will be undrafted when #1036 is closed.
Commit tmp: temporary fixes to api/hash will not be included in the final version of this PR.
Description
sda.files
tablesda.encryption_keys
, an error is thrown and ingest stopsHow to test
Add the hashed public key to
sda.encryption_keys
and then try to ingest a file. See the integration test for details.