Feature/validator orchestrator #2057
Conversation
KarlG-nbis
force-pushed
the
feature/validator-orchestrator
branch
from
dfbe13f to
f79cb50
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2057 +/- ##
==========================================
- Coverage 44.19% 43.64% -0.56%
==========================================
Files 49 48 -1
Lines 6956 6849 -107
==========================================
- Hits 3074 2989 -85
+ Misses 3504 3491 -13
+ Partials 378 369 -9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
KarlG-nbis
force-pushed
the
feature/validator-orchestrator
branch
6 times, most recently
from
3a4c6fe to
c4183a2
Compare
jbygdell
left a comment
jbygdell
left a comment
There was a problem hiding this comment.
There are so many thing here that goes against my normal coding principles so i don't know what to do.
More specifically:
- the use global/shared variables
- naked functions (no return values)
- functions that return functions or function wrapped values
Which is maybe why the use of the init() function never has made much sense to me. I prefer implicit calls for that kind of things as it makes for fewer gotcha moments.
Outside of that it looks like a nice proof of concept.
KarlG-nbis
force-pushed
the
feature/validator-orchestrator
branch
from
24794d1 to
2fb253d
Compare
KarlG-nbis
force-pushed
the
feature/validator-orchestrator
branch
2 times, most recently
from
4854f60 to
1838171
Compare
…/rbac yet), an apptainer example and kube files
… files, and override help func with func that also prints env varible name
…itive complexity with big functions
… for unit testing, add optional config for setting the work dir of validation of files
…, fix some spacing items, fix typos, add check if file paths requested was found to belong to user
…mpressed sandbox build
| if err := internalconfig.Load(); err != nil { | ||
| log.Fatalf("failed to load config due to: %v", err) | ||
| } | ||
|
|
||
| if err := validators.Init(config.ValidatorPaths()); err != nil { | ||
| log.Fatalf("failed to initialize validators due to: %v", err) | ||
| } | ||
|
|
||
| amqpBroker, err := broker.NewAMQPBroker() | ||
| if err != nil { | ||
| log.Fatalf("failed to create new AMPQ broker due to: %v", err) | ||
| } | ||
|
|
||
| if err := postgres.Init(); err != nil { | ||
| log.Fatalf("failed to initialise postgres database due to: %v", err) |
There was a problem hiding this comment.
There are many Fatalf, better to return errors and shutdown gracefully.
There was a problem hiding this comment.
This is before there is any reason to shutdown gracefully, and eg if broker connection can not be started we do not want to init the pg just to then close it again
| mockCommandExecutor *mockCommandExecutor | ||
| } | ||
|
|
||
| func (ts *ValidatorsTestSuite) SetupTest() { |
There was a problem hiding this comment.
Since there is Init() in validators.go, Init() will be called every time when a test function is run, and therefore, test may pollute each other. Better to reset the global states for validators at the SetupTest(), e.g. Validators = make(map[string]*ValidatorDescription)
There was a problem hiding this comment.
Not sure I follow the concern with the init() func, but will add a TearDownTest() to reset the global states after each test
nanjiangshu
left a comment
nanjiangshu
left a comment
There was a problem hiding this comment.
Great work @KarlG-nbis! The framework looks nice and and overall logic looks fine to me.
I looked at the README, orchestrator main code, the api and Apptainer parts carefully and left some comments. But for the rest components I haven't reviewed carefully. However, I think we can improve them later on when we go beyond this proof-of-concept.
…nFileSize, add unit test, add database function to set files submission_file_size
…atabase from outside database package
…mi random number for TestSetSubmissionFileSize unit test
…http server with TLS, update main.go to react to ctx cancellation if dependants have unexpected error instead of sending of chan for os signals, fix some issues in unit tests
…ate of global(package) variables after each test
Related issue(s) and PR(s)
This PR closes #990.
Description
Implementation of the sda-validator-orchestrator which is responsible for orchestrating the validation of files according to the Validator API specification
How to test