migration to go mod, moving from standard library http handler to echo

[joicemjoseph]

@navaneeth please review

[joicemjoseph]

Good to go

[subins2000]

I don't know Go, is echo a HTTP library like express in nodejs ? Would it be possible to add an authentication to only allow some people to add words with /learn endpoint ?

[joicemjoseph]

Yes, It is possible. We can do it without any extra libraries. They have a middleware. We can do the same without any third party library. On request, encode username and password in base64 format and send it on Authentication header, decode and string match credentials and can proceed.

[subins2000]

I tried the branch, but getting this error on learn handler.

curl 'http://localhost:8080/learn' -H 'Origin: http://localhost:8080' -H 'DNT: 1' -H 'Connection: keep-alive' -H 'Referer: http://localhost:8080/' --data-raw $'{"text":"\u0d15\u0d4d","lang":"ml"}'

gives :

{"message":"unable to find language"}

[subins2000]

Running varnamd -enable-download ml,hi -sync-interval 1 doesn't seem to do anything too. Idk what's the expected working @navaneeth

[joicemjoseph]

I tried the branch, but getting this error on learn handler.

curl 'http://localhost:8080/learn' -H 'Origin: http://localhost:8080' -H 'DNT: 1' -H 'Connection: keep-alive' -H 'Referer: http://localhost:8080/' --data-raw $'{"text":"\u0d15\u0d4d","lang":"ml"}'

gives :

{"message":"unable to find language"}

try curl -XPOST 'localhost:8080/learn' -d '{"word":"\u0d15\u0d4d","lang":"ml"}' -i -H 'Content-Type: application/json'

PS: Make sure you have mentioned content-type

[subins2000]

Didn't work, same error happens. Can you try it out in varnamd's UI and see if it works on your side ?

[joicemjoseph]

Didn't work, same error happens. Can you try it out in varnamd's UI and see if it works on your side ?

There was an issue, which I mistaken for a json element key. I resolved it. But I found out that request's content type is not 'application/json' but 'form-url-encoded'.

The request is made from https://github.com/varnamproject/varnamd/blob/master/ui/javascripts/varnam.js#L77 It is in the minified form. @subins2000 could you send me a patch by adding content type as 'application/json' ?

[joicemjoseph]

moved to draft as I found a bug in download content encoding

[subins2000]

Can confirm, learn endpoint now gives a "success" response. Thanks for the fix!

[subins2000]

Um, the config file, the new libvarnam folder and stuffbin is not applicable for upstream varnamd, right ? I think it's unnecessary to be in varnamd, the server because all these changes are made for varnam-desktop to work.

[joicemjoseph]

stuffbin will help to ship the binary, deploy it easily. there wont be any possible file missing on deployment.

I think it is better to have file based configuration instead of flags based.

[subins2000]

For a server deployment it makes sense, but varnamd is used by ibus-engine-varnam. That doesn't require the ui files. So bundling the ui is unnecessary for varnamd.

[subins2000]

ibus-varnam spawns varnamd with the -p port flag.

If these new changes (with the new config.toml) get merged, it'll break ibus engine. One workaround is to override the flags set in config.toml file with CLI args if it's available.

[joicemjoseph]

There is a default value in varnamd, without any config file, it will run on port 8080.

as a workaroud, we can add env variable as well as config flag.

(Thanks for pointing out ibus-varnam's varnamd dependency. I forgot that)

[subins2000]

Perhaps allow usage of flags that'll override the config file ?

[joicemjoseph]

Perhaps allow usage of flags that'll override the config file ?

1f62ad1 resolved this

[navaneeth]

There are so many changes. Not sure I have gone through and tested fully. @joicemjoseph , @subins2000 tested this?

[joicemjoseph]

Yep. There are a lot of changes. Yet more changes are on pipeline. We have tested. But, Subin have made some PR to my fork. I will merge them. Then we can merge this PR. How that would be. ??

[navaneeth]

@joicemjoseph Sure. That makes sense

[subins2000]

@navaneeth This PR is now complete. It'd be great if you can give org access to both of us so that we can work straight on varnamd (with PRs of course) skipping the fork of a fork changes.

[subins2000]

We've been running this changed varnamd here and on the desktop app. 90% of these changes have been tested and works well.

Recommending this to be merged.