-
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
Added possibilty to register publisher; Automatic creation of DNS-Server implemented; Domain-managemend partially implemented #144
base: master
Are you sure you want to change the base?
Conversation
…ver implemented; Domain-managemend partially implemented
…rom dns server correctly; Remove Urls from publishers; Add Urls to publishers
…ned to a journal, it will run with the corresponding DNS Server
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.
Just a few comments and questions - consider everything optional, writing is more important.
@@ -148,7 +152,6 @@ c.bagtainer.docker.create_options = { | |||
Env: ['O2R_MUNCHER=true'], | |||
Memory: 4294967296, // 4G | |||
MemorySwap: 8589934592, // double of 4G | |||
NetworkDisabled: true, |
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 have to admit this does make me a little bit nervous :-)
"cache-size=100000\n"; | ||
c.dns.dnsmasq.filterDummy = "server=/"; | ||
c.dns.priority.publisher = 100; | ||
c.dns.priority.journal = 50; |
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.
Gibt es hier irgendwas das ein Plattformbetreiber Konfigurieren wollen würde, oder ist das ziemlich stabil? Wenn Konfiguration, dann gerne das Muster mit den Umgebungsvariablen nutzen wie an anderen Stellen in dieser Datei.
}); | ||
} | ||
|
||
exports.getJournal = function (req, res) { |
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.
Hier lohnt sich ein kurzer Kommentar im Quelltext - dass es sowohl viewJournal
als auch getJournal
gibt finde ich nämlich im ersten Momennt verwirrend. Die Unterscheidung auf Basis des user levels könntest du auch mit einer Funktion machen.
Sind das auch verschiedene Endpunkte in der API?
app.get('/api/v1/publisher/:id/journals', controllers.publisher.getPublisherJournals); | ||
app.put('/api/v1/publisher/:id/update', controllers.publisher.update); | ||
app.put('/api/v1/publisher/:id/adddomain', controllers.publisher.addDomain); | ||
app.put('/api/v1/publisher/:id/removedomain', controllers.publisher.removeDomain); |
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 not app.delete("/api/v1/publisher/:id/domain/:domainId", ...)
?
app.get('/api/v1/publisher/:id/domains', controllers.publisher.getPublisherDomains); | ||
app.get('/api/v1/publisher/:id/journals', controllers.publisher.getPublisherJournals); | ||
app.put('/api/v1/publisher/:id/update', controllers.publisher.update); | ||
app.put('/api/v1/publisher/:id/adddomain', controllers.publisher.addDomain); |
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.
"Add" = new resource > usually an HTTP POST operation.
id: String, | ||
name: String, | ||
domains: [String], | ||
owner: String, |
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.
A DNS can have multiple owners - why not a journal, too?
name: String, | ||
domains: [String], | ||
journals: [String], | ||
owner: String, |
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 can't a publisher have multiple owners?
'versioning'] | ||
|
||
f = open('re3data-metrics.json', 'w') | ||
f.write('{') |
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.
Huh... seems a bit hacky, but if it works... you know you can easily serialise a Python dictionary to JSON?
@@ -966,6 +966,7 @@ function save(passon) { | |||
var compendium = new Compendium({ | |||
id: passon.id, | |||
user: passon.user, | |||
journal: '', |
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.
Might be better to set the default in the model - see comment above.
@@ -37,6 +37,7 @@ | |||
"erc-checker": "o2r-project/erc-checker#v1.3.0", | |||
"express": "^4.17.1", | |||
"express-session": "^1.17.0", | |||
"fast-xml-parser": "^3.19.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.
That's all? Nice!
|
No description provided.