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

[WIP] Feat/dockerize #14

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[WIP] Feat/dockerize #14

wants to merge 4 commits into from

Conversation

frani
Copy link

@frani frani commented Oct 14, 2024

Hi @pennersr ,

I’m adding a Docker option, as I believe it could be useful, especially for using Docker Compose (which I use frequently).

I’m also including the option to read "vapid-keys" from a file.

The branch is still a work in progress, but I wanted to open a PR so we can discuss it.

frani added 4 commits October 14, 2024 15:52
- fix typo var name webPushVAPIDKeysJSON
- priorise flag than file
- update to go 1.23.1
- fix go build
- use command to start container
Copy link
Owner

@pennersr pennersr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing! I've added a few comments...

Start the server:

$ shove \
-api-addr localhost:8322 \
-queue-redis redis://redis:6379 \
-fcm-credentials-file /etc/shove/fcm/credentials.json \
-apns-certificate-path /etc/shove/apns/production/bundle.pem -apns-sandbox-certificate-path /etc/shove/apns/sandbox/bundle.pem \
-webpush-vapid-public-key=$VAPID_PUBLIC_KEY -webpush-vapid-private-key=$VAPID_PRIVATE_KEY \
-webpush-vapid-key-file=/etc/shove/webpush/vapid-keys.json \
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This uses -key- whereas above it says -keys-.

-telegram-bot-token $TELEGRAM_BOT_TOKEN


Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For readability, I would prefer to keep 2 newlines before starting a new section.

@@ -37,7 +38,8 @@ var redisURL = flag.String("queue-redis", "", "Use Redis queue (Redis URL)")
var webhookWorkers = flag.Int("webhook-workers", 0, "The number of workers pushing Webhook messages")

var webPushVAPIDPublicKey = flag.String("webpush-vapid-public-key", "", "VAPID public key")
var webPushVAPIDPrivateKey = flag.String("webpush-vapid-private-key", "", "VAPID public key")
var webPushVAPIDPrivateKey = flag.String("webpush-vapid-private-key", "", "VAPID private key")
var webPushVAPIDKeysJSON = flag.String("webpush-vapid-keys-json", "", "JSON file containing VAPID keys")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var webPushVAPIDKeysJSON = flag.String("webpush-vapid-keys-json", "", "JSON file containing VAPID keys")
var webPushVAPIDKeysFile = flag.String("webpush-vapid-keys-file", "", "JSON file containing VAPID keys")

(that's consistent with e.g. -fcm-credentials-file)


if *telegramBotToken != "" {
} else if *webPushVAPIDKeysJSON != "" {
content, err := os.ReadFile(*webPushVAPIDKeysJSON)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to add a check here that both webPushVAPIDPublicKey and webPushVAPIDPrivateKey are empty.

slog.Error("Failed to unmarshal WebPush VAPID keys file, please check your JSON file containt 'publicKey' and 'privatekey' keys", "error", err)
os.Exit(1)
}
web, err := webpush.NewWebPush(vapidKeys.PublicKey, vapidKeys.PrivateKey, newServiceLogger("webpush"))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need to check here that after the unmarshal public key and private key are really set?

slog.Error("Failed to add WebPush service", "error", err)
os.Exit(1)
}
} else if *telegramBotToken != "" {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue: This should be an else if -- Telegram is independent from the webpush config.

@@ -0,0 +1,40 @@
ARG GO_VERSION=1.23.1
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you split this into two separate MRs? One for docker related functionality, the other for the vapid keys file.

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.

2 participants