Skip to content
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

Closed
wants to merge 29 commits into from
Closed

8 rebased rest service #19

wants to merge 29 commits into from

Conversation

phwissmann
Copy link
Collaborator

No description provided.

phwissmann and others added 27 commits September 4, 2024 13:11
* 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
This should be restricted more in the future (eg with a `--host` config
param to specify the frontend url)
@consolethinks consolethinks marked this pull request as ready for review September 27, 2024 14:23
@consolethinks consolethinks self-requested a review September 27, 2024 14:23
api/openapi.yaml Outdated
Copy link
Collaborator

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

internal/task/task.go Outdated Show resolved Hide resolved
Copy link
Collaborator

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

Copy link
Collaborator Author

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.)

Copy link
Collaborator

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.

Copy link
Collaborator Author

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

internal/core/taskqueue.go Outdated Show resolved Hide resolved
internal/core/globus.go Outdated Show resolved Hide resolved
@consolethinks
Copy link
Collaborator

Note that I'm fine with merging this as is, just added a few highlights with the review above

internal/core/taskqueue.go Outdated Show resolved Hide resolved
internal/task/api.go Outdated Show resolved Hide resolved
internal/task/task.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants