Ingest adds keyhash to files db table #1073
Conversation
MalinAhlberg
force-pushed
the
feature/ingest-add-keyhash
branch
5 times, most recently
from
ecec008
to
ceecb05
Compare
kostas-kou
force-pushed
the
feature/api-keys
branch
from
1cfac07
to
5fc3703
Compare
|
This PR should not have to be dependent on #1036 |
kostas-kou
force-pushed
the
feature/api-keys
branch
from
9c42595
to
f6ab0a7
Compare
MalinAhlberg
force-pushed
the
feature/ingest-add-keyhash
branch
6 times, most recently
from
ca919ba
to
910a2db
Compare
jbygdell
force-pushed
the
feature/api-keys
branch
from
cba4dff
to
97893e3
Compare
MalinAhlberg
force-pushed
the
feature/ingest-add-keyhash
branch
from
910a2db
to
b6b5fcd
Compare
jbygdell
force-pushed
the
feature/api-keys
branch
5 times, most recently
from
440b590
to
b488b73
Compare
MalinAhlberg
force-pushed
the
feature/ingest-add-keyhash
branch
from
85f4ef9
to
0f68dbe
Compare
There was a problem hiding this comment.
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.
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.
| if [ "$key_hashes" -lt 0 ]; then | |
| if [ "$key_hashes" -eq 0 ]; then |
There was a problem hiding this comment.
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.
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.
Why ignore the potentially returned error message?
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.filestablesda.encryption_keys, an error is thrown and ingest stopsHow to test
Add the hashed public key to
sda.encryption_keysand then try to ingest a file. See the integration test for details.