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

Relax sync.server.origin to allow paths #3423

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

Conversation

chromy
Copy link

@chromy chromy commented May 1, 2024

Hi! How would you feel about relaxing the restriction that sync.server.origin must be protocol + domain (https://tcsync.example.com) and allow paths also (https://example.com/tcsync/)?

This is useful for me since I run a bunch of services on a single server behind nginx:

proxy_set_header X-Forwarded-For $remote_addr;
location /calibre/ {
    proxy_buffering off;
    proxy_pass http://127.0.0.1:8080$request_uri;
}
location /calibre {
    # we need a trailing slash for the Application Cache to work
    rewrite /calibre /calibre/ permanent;
}

# Web UI for taskwarrior
location /tasks-chromy/ {
    proxy_pass http://127.0.0.1:8001$request_uri;
}

# root page
location / {
    root /usr/share/nginx/html;
    try_files /index.html =404;
}

# ...etc

Previously I ran taskd on its dedicated port (53589) which meant I had to configure this ports to be open etc which was always a little worrying. Now with the advent of taskchampion-sync-server I can very almost run it behind port 80/443:

location /taskchampion/ {
    proxy_pass http://127.0.0.1:7999/;
}

with exception of this check. Just removing that was sufficient but I have beefed up the error handling a little and added some unittests while I was here.

I appreciate this is a little niche ...but running ones own taskchampion-sync-server instance is already fairly niche so maybe it won't so uncommon given the selection effect ;)

Description

Previously sync.server.origin had to be a scheme + FQDN, for example: https://tcsync.example.com
After this we support paths in sync.server.origin, for example: https://example.com/tcsync/
If a path is present it must end with a slash.

This allows for taskchampion-sync-server run behind e.g. nginx on port 80 / 443 at the same time as other services. This is desirable since:

  • It avoids the need to choose between a. configuring additional non-standard ports or b. having dedicated HW/VMs/DNS routes just for taskchampion.
  • Increases the flexibility of taskchampion.

Also:

Additional information...

  • I changed C++ code or build infrastructure.
    Please run the test suite and include the output of cd build/test && make && ./problems.

  • I changed Rust code or build infrastructure.
    Please run cargo test and address any failures before submitting.

Previously sync.server.origin had to be a scheme + FQDN, for example:
https://tcsync.example.com
After this we support paths in sync.server.origin, for example:
https://example.com/tcsync/
If a path is present it must end with a slash.

This allows for taskchampion-sync-server run behind e.g. nginx on port
80 / 443 at the same time as other services. This is desirable since:
- It avoids the need to choose between a. configuring additional
  non-standard ports or b. having dedicated HW/VMs/DNS routes just for
  taskchampion.
- Increases the flexibility of taskchampion.

Also:
- Error on some degenerate URLs (mailto:tcsync@example.com)
- Handle sync.server.origin = https://tcsync.example.com?foo=bar more
  correctly. Previously we geneated invalid URLs of the form:
  https://tcsync.example.com?foo=barv1/foo/bar.
- Add some unittests for the various cases
- Fix a typo impelementations -> implementations.
@ryneeverett
Copy link
Contributor

This appears to be implementing #3414.

@djmitche djmitche self-requested a review May 1, 2024 17:20
@djmitche
Copy link
Collaborator

djmitche commented May 1, 2024

This appears to be implementing #3414.

It is! Awesome! @davidhelbig already landed the Taskchampion part of this in GothenburgBitFactory/taskchampion#367, so what remains is #3209 (which will remove much of the taskchampion code modified here) and then the Taskwarrior part of this work, which is present in this PR.

So, if you don't mind waiting a bit for me to finish up #3209 and then rebasing this, that will finish the work! I've been trying to get to #3209 but honestly have been spending all my available time replying to issues. I have a few hours to myself tonight so hopefully can get it done during that time.

@djmitche
Copy link
Collaborator

djmitche commented May 2, 2024

OK, #3209 has landed! We'll need to make another TaskChampion release to depend on from here, but it's late :)

However, I see that this PR doesn't change the name of the config option. An origin doesn't, by definition, have a path, and I think we should be consistent about that naming, as mentioned in #3414. One the new version of TaskChampion is out, would you be willing to make an update to TaskWarrior to use a new config name (with compatibility with the old)?

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.

None yet

3 participants