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

New checkbox to allow for connections via SSL #80

Merged
merged 6 commits into from
Sep 25, 2024
Merged

Conversation

Cinzya
Copy link
Contributor

@Cinzya Cinzya commented Sep 8, 2024

I am running ontime behind a proxy which only allows connections through SSL.
So I added another checkbox to the config, which is by default false, to check if SSL is prefered. In the connection phase, it will then either use ws or wss depending on the state of the checkbox, and either http or http when fetching custom fields and all events.

grafik

@alex-Arc
Copy link
Contributor

Hey @Cinzya looks good
Could you also bump the minor version

@Cinzya
Copy link
Contributor Author

Cinzya commented Sep 14, 2024

Bumped version to 4.4.0 in package.json and manifest.json.

@@ -46,7 +46,9 @@ export function connect(self: OnTimeInstance, ontime: OntimeV3): void {
host.replace(pattern, '')
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @Cinzya I've tested you stuff in a full stack and it works great

but i then found that this replace dosn't work so if you put in https://ontime.dk into the Ontime server address field it would fail with Connection Failure WebSocket error: getaddrinfo ENOTFOUND https

would you have time to look at this?

It is not directly related to your PR but it would fit in ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I wasn't aware that this is an option.

Seems to be an easy fix, I should be able to take a look at it later today or in the next few days. No problem!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

found the issue and pushed a fix!

@alex-Arc
Copy link
Contributor

@cpvalente if you would give it a quick look, then it is ready for merging

@cpvalente
Copy link
Collaborator

Thank you both @Cinzya and @alex-Arc , great work!

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