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

Support subpath in url for Nightscout follower #3089

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jeroennijhof
Copy link

This will add support for subpaths in the Nightscout follower url.

Currently when you have a Nightscout instance running at i.e. https://domain.com/subpath/ it will strip the /subpath/ from the url which is not correct. By using relative paths for the @get methods subpaths are working.

@Navid200
Copy link
Collaborator

Is there any Nightscout server that cannot produce a hostname without a subpath?

@jeroennijhof
Copy link
Author

I'm not sure what you mean but the library used, retrofit, makes sure the baseurl will always end with a slash so it will be safe to use the relative path.

@Navid200
Copy link
Collaborator

Nightscout needs a server.
If you use Heroku, or Google Cloud or anything else I am aware of, the hostname will be something like thgis:

user.heroku.com

or

user.chickenkiller.com

for example.

I am asking why would anyone have a subpath?
If everyone can create a hostname that looks like user.heroku.com, which has no subpath, why would we need to worry about a subpath?

@jeroennijhof
Copy link
Author

Nightscout needs a server indeed, but since it's open source you can decide yourself how you host it right? I'm not really sure why you want to restrict choices?

@Navid200
Copy link
Collaborator

The reason xDrip still exists and is supported is that a few volunteers have spent a lot of time on it and continue to support it.
It makes no sense to support something that is not absolutely needed considering the limited resources we have.
We have open issues because we don't have enough developers to fix them.
Some come here and open a PR and after it is merged, we never see them again. Then, something breaks, and we have to reverse engineer to figure out what is causing the problem.
We cannot approve every single PR without asking questions.

Is there a reason you cannot set up your path as user.domain.com without a subpath?
If you can, you will not need this PR, right? Then, it is not a necessity.
I could be wrong about this. But, you are not helping me see why.

@Navid200
Copy link
Collaborator

I would like to ask my question differently to hopefully help clarify my point in case I am not clear already.

I understand that if there is a hostname that contains a subpath, currently, xDrip has a problem and this PR will resolve it. I'm not disputing that.

But, I wonder why there should be any hostname that contains a subpath.

1- If someone has the option to choose a hostname with or without a subpath, why would they choose to have a subpath? What advantage would it offer over a simple hostname with no subpath?

2- Is there a scenario where a user has no choice and for whatever reason they have to have a subpath? What is that scenario?

3- It is possible that someone has already created a Nightsocut setup and have given you a hostname and you want to follow them. But, the hostname they have given you contains a subpath. Is that the case? Would you please ask them why they chose a subpath?

I would just like to get a sense for how important this is and if possibly there may be a workaround.

@psonnera
Can you shed any light on this?

@psonnera
Copy link

Whilst this change, if tested correctly, doesn't look bad at all, I believe the community would benefit having more information on the Nightscout deployment method and use case in question. Subpath probably allows reusing an existing DNS for either more apps or Nightscout instances. @jeroennijhof is that the case?

@jeroennijhof
Copy link
Author

@psonnera that's correct. We currently run 5 Nightscout instances and since we already had a ssl certificate, domain and vps it's pretty easy and cheap to run it all on one vm. Sure I can set it up with subdomains but that means I need to buy 5 ssl certificates to be more secure. I don't like to expose the api secrets on http. And since I run into this I figured there will be more people out there wanting to reuse their VM, DNS and SSL.

@Navid200 thank you for your explanation I really understand the pain since I run a couple of o.s. projects myself so if you want to close the PR, that's fine.

@Navid200
Copy link
Collaborator

No, I don't want to close the PR.
I ask questions in some PRs. But, I am not the one who makes the last call. Having had this conversation, my hope is that it addresses some questions that jamorham would have had to ask anyway. So, my hope is that asking these questions helps him.

Thanks

Let's now wait for his response.

@jamorham
Copy link
Collaborator

What are the pro's and cons of merging this? Can it break any current user's settings?

@Navid200
Copy link
Collaborator

Navid200 commented Sep 23, 2023

I am embarrassed to say I jumped in and asked questions without looking at the literal changes. Just 2 slashes?!
I'm sorry.

We just need to see test results.

@jamorham
Copy link
Collaborator

A single character is enough cause serious problems. This change maps the API to a relative instead of absolute root url. There is a very strong chance that it works but I would like to ensure the pros and cons have been tested to avoid us breaking any existing installations that might be affected by this. I would like to hear from @jeroennijhof if they think there could be an unintended consequences for existing users with this change?

@jeroennijhof
Copy link
Author

@jamorham the only way this change could have any unintended issues for people when they already have a subpath in the Nightscout url i.e. with the api full path. Then the url will become invalid as in /api/v1/api/v1/entries or sorts. But since the majority have Nightscout running under it's own domain name I think it's not a really big issue, but I'm not sure.

@@ -212,4 +212,4 @@ public static void resetInstance() {
UserError.Log.d(TAG, "Instance reset");
CollectionServiceStarter.restartCollectionServiceBackground();
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please do not remove the EOF line break!

@tolot27
Copy link
Collaborator

tolot27 commented Jun 13, 2024

@jamorham the only way this change could have any unintended issues for people when they already have a subpath in the Nightscout url i.e. with the api full path. Then the url will become invalid as in /api/v1/api/v1/entries or sorts.

Can this be detected and removed from the URL setting?

A unit test would be great to test against this potential problem. Afterwards a fix can be implemented, i. e. by removing the API subpath. This is at least necessary for #1765.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants