-
Notifications
You must be signed in to change notification settings - Fork 0
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
8 rebased rest service #19
Conversation
* Add repo skeleton and desktop-app framework Add wails.io based app with svelte frontend, a taskqueue and basic go module and package structure * Follow standard go project structure * Add CI workflow * App: Don't fail when config is missing * Add linting * Fix defer * Abstract progress notifier Removes dependency of taskqueue on wails * Move core package into separate folder
* start adding globus components * add globus authentication and transfer request * simplify tasks * add user data extraction and implement info verification * add path prefixes to globus transfer * config handling changes * go.sum changes * hackish changes * upgrade globus dependency * error handling and globus client init * work around viper issue with shashlik case * fix date creation, add function for checking globus client ready-ness * add scicat-cli patch package temporarily * fix channel timers for globus ingestion, move notification handling to main "GlobusTransfer" function * remove unused parameter from "globusCheckTransfer" * Add goreleaser to ci pipeline and add config file (#12) * update ci * add more fixes * add even more ci fixes * fix go linting * add dist filler file for linting * add filler file --------- Co-authored-by: phwissmann <33758534+phwissmann@users.noreply.github.com>
Disable bearer auth for development
Create docs from function annoations to be served under /docs/index.html
api/openapi.yaml
Outdated
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 API will need better naming conventions and better request and response schemas, but for now it's alright. @phwissmann @dwiessner-unibe
cmd/openem-ingestor-service/main.go
Outdated
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 headless rest service should now be functional. Though I'd somewhat prefer to not have to create a dummy notifier for this (maybe some way to just pass a 'nil' then check whether there's a notifier?), but it's not a big issue. Oh also, I haven't yet removed the REST service process from the GUI app, I'll leave that up to you @phwissmann
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 bit unclear to me, what should be shown in the ingestor UI in terms of progress/status. Just current status or also the upload progress? If also the progress, I would suggest to use a websocket to stream the progress which can be implemented using the notifier interface.
Instead of making the dummy notifier you could make the notifier optional in the taskqueue, or even better, make a logging notifier so we see in the logs what happened (I think that was my idea, otherwise there is no output when a task fails.)
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.
okay, so the globus transfer can only poll for status updates on the transfer, so the progress only gets updated every once in a while (currently set to 1 minute but that can be changed to more frequent polling). Not sure if a websocket is important when considering this limitation.
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 added structured logging for the tasks and removed the dummy notifier. We get such output in the console
2024/10/14 15:19:03 INFO Task added id=1cb95267-4a53-40dc-9d37-22794fbb71e0 folder=/home/wiphilip/Documents/projects/openemnetwork/src/Ingestor/cmd/openem-ingestor-app/build/bin
Scheduled upload for: {1cb95267-4a53-40dc-9d37-22794fbb71e0 /home/wiphilip/Documents/projects/openemnetwork/src/Ingestor/cmd/openem-ingestor-app/build/bin}
2024/10/14 15:19:06 INFO Task scheduled id=1cb95267-4a53-40dc-9d37-22794fbb71e0
2024/10/14 15:19:14 ERROR Task failed id=1cb95267-4a53-40dc-9d37-22794fbb71e0 err="scicat: couldn't get user info from token: Get \"http://scicat:8080/api/v3/users/my/self\": dial tcp: lookup scicat on 127.0.0.53:53: server misbehaving"
scicat: couldn't get user info from token: Get "http://scicat:8080/api/v3/users/my/self": dial tcp: lookup scicat on 127.0.0.53:53: server misbehaving
Note that I'm fine with merging this as is, just added a few highlights with the review above |
f454421
to
778a057
Compare
No description provided.