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

Patch: Fix URL path, WG subnet and initsetup route logic #196

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

lifehome
Copy link

@lifehome lifehome commented Jun 20, 2021

cc: @subspacecommunity/subspace-maintainers
resolves: #195, also resolves #158

Background

Changes

  • Prepended all template links with a subdirectory URL path
  • Reworded "backlink" to either "subdir"/"SubDirectory"/"URL_SUBDIRECTORY" accordingly
  • Fixed WireGuard configuration by utilizing the subnet provided by user
  • Fixed Subspace peer deletion redirection to a 404 error page

Testing

The changes have been baked into a Docker image locally, it is up and running healthily with a WireGuard server installed on the host. All pages and functions are tested, except SAML and IDP related features/pages/functionalities.

lifehome added 25 commits June 21, 2021 02:56
This is in case the subdir variable is empty, which could render the navigation link href empty.
@sonarcloud
Copy link

sonarcloud bot commented Jun 20, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

No Coverage information No Coverage information
3.2% 3.2% Duplication

@gchamon
Copy link

gchamon commented Aug 8, 2021

Sorry for the long wait. I will get to testing it with saml. However, it has been long since I could last get I touch with the other mainteiners, not sure I could merge it without a second peer review.

On a related note, would you be interested in getting more involved in the project? We sure could use extra hands for pr reviews.

@lifehome
Copy link
Author

lifehome commented Aug 8, 2021

Glad to help!

entrypoint.sh Outdated Show resolved Hide resolved
@lifehome lifehome requested a review from gchamon October 9, 2021 07:53
It seems the httprouter cannot handle index path without trailing slash
@lifehome lifehome marked this pull request as draft October 9, 2021 08:37
Check and redirect, instead of creating a redirect loop.
@lifehome lifehome marked this pull request as ready for review October 9, 2021 09:11
@lifehome
Copy link
Author

The PR should be ready for review and merge, please kindly take a look. Thanks!

PSA: I might only do maintenance PR for this repository, as I plan to rewrite the concept of subspace into a WireGuard backend RESTful API like ZeroTier One FOSS Controller, and also a revamped frontend for it. Might need insights and testers for the SAML/SSO part tho...

ping @gchamon @jfrede @agonbar
(Sorry for the ping but it seems I cannot tag the contributor team in this repo)

Copy link

@gchamon gchamon left a comment

Choose a reason for hiding this comment

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

Other than that, I wanted to find some way to cleanly set the subdirectory (I am not saying there is, only that I haven't looked into it).

I don't feel that much comfortable with the amount of repetition we need to employ to prepend subdir to all the routes. Any maintenance or added feature has to keep this in mind in order to support the feature.

Maybe the router module that is used could provide a cleaner way to set this kind of path subdir, but I would have to look into it.

Dockerfile Outdated Show resolved Hide resolved
@lifehome
Copy link
Author

The repetition is an intended implementation to work around the (at least to me) unmaintained http router package.

The julienschmidt/httprouter#89 was raised in 2015, updated on 2017 and until now none of the maintainers of that package gives a look...

PS: These are just all wild guess in my head... Looking on their profile, I doubt they can contribute in FOSS domain as usually employment contracts prohibits such contributions or Intellectual Properties that doesn't owned by the company.

@lifehome lifehome requested a review from gchamon October 12, 2021 08:05
@sonarcloud
Copy link

sonarcloud bot commented Oct 16, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

No Coverage information No Coverage information
3.1% 3.1% Duplication

@lifehome
Copy link
Author

lifehome commented Oct 16, 2021

Patched and it seems all is good... Right?

ping @gchamon @jfrede @agonbar for another review.
(Sorry for the ping but it seems I cannot tag the contributor team in this repo)

EDIT on Oct 24 2021: Sorry but it seems I mistakenly replayed a headless action, sorry for the repeated ping. 😭

@lifehome
Copy link
Author

Gently push a bit.

@gchamon
Copy link

gchamon commented Nov 20, 2021

Hey there, sorry I didn't answer. I can't get in contact with other maintainers to discuss this pull request. Since there is interest in this pr from other users, if other maintainers approve I have no objections to this pr

@lifehome
Copy link
Author

Can we fork away this repository from the closed loop? Unless the constraint of requiring approval from two contributors with write access, this repository cannot live...

@gchamon
Copy link

gchamon commented Nov 23, 2021

The best thing would be to have more volunteers reviewing pull requests, but a fork is always possible. I thought of creating a "nightly" branch here with more permissive constraints and replicating pull requests there and creating a separate taggins system for docker. Another problem is that I am still without access to the dockerhub repo to fix and maybe extend the current tagging system.
So yeah, the easiest would be a fork.

@gchamon
Copy link

gchamon commented Nov 23, 2021

We use a fork where I work. There I tested the saml dep upgrade and other interesting improvements before they went mainstream. It needs to be synced with this repo, but we could use that in the meantime while we hopefully find other volunteers to maintain this repo

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.

Bug Report: Absolute path on assets and links docker-compose SUBSPACE_IPV4_POOL= ignored?
2 participants