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

Custom flags for tailscale up & Misc fix #12

Merged
merged 19 commits into from
Feb 13, 2024

Conversation

SaigyoujiYuyuko233
Copy link
Contributor

@SaigyoujiYuyuko233 SaigyoujiYuyuko233 commented Jan 21, 2024

Relate to issue: #11

This PR will:

  • allow user to set custom Tailscale login server --login-server=
  • allow user to set custom Tailscale operator --operator=
  • Some quanlity of life improvements

Here is how it looks like:
image
image

@SaigyoujiYuyuko233 SaigyoujiYuyuko233 changed the title Add custom login server support Add setting for tailscale up Jan 21, 2024
@saumya-banthia
Copy link
Owner

saumya-banthia commented Jan 23, 2024

@SaigyoujiYuyuko233 thanks for the feature addon, I'm currently away from home, will review code as soon as I'm back, revert if something requires change and then merge.

Just an initial observation based query: Any reason to create a modal popup for the settings you've added (instead of how other settings currently are)?

@SaigyoujiYuyuko233
Copy link
Contributor Author

@SaigyoujiYuyuko233 thanks for the feature addon, I'm currently away from home, will review code as soon as I'm back, revert if something requires change and then merge.

Just an initial observation based query: Any reason to create a modal popup for the settings you've added (instead of how other settings currently are)?

For some unknow reason, in my deck (running nobara instead of steamos), the virtual input method does not popup when I add TextInput in quick access. The ip input in Decky Developer -> Remote React Developtools also has the same issue.

And I think popup is just wider and bigger, so it is easier for user to type and view after they finish editing.

@SaigyoujiYuyuko233 SaigyoujiYuyuko233 changed the title Add setting for tailscale up Custom flags for tailscale up Jan 26, 2024
@SaigyoujiYuyuko233
Copy link
Contributor Author

SaigyoujiYuyuko233 commented Jan 26, 2024

I kept the login server input because I think this entry is longer than other and it is easier to edit with a dedicate input box.
update:
image

… script error spamming on system journal

because tailscaleToggleState does not update dynamically
@SaigyoujiYuyuko233 SaigyoujiYuyuko233 changed the title Custom flags for tailscale up Custom flags for tailscale up & Misc fix Jan 26, 2024
@saumya-banthia
Copy link
Owner

Will take a look at this starting the next week, I do want to ensure a few things:

  • Adding new functionality, should not break previously working stuff (meaning people should be able to opt-in not opt-out of additional config setup) - this also prevents default config errors.
  • Another consideration is to not build a monolith, as I think a 3rd party plugin can/should go only so far, given how just one change can render previously usable things unusable.
  • Easily maintainable (more so, for this being a hobby project).

None of these are specific to any code you have written, as I haven't found time to review code so far. I just want to make sure it goes by the same general rules of the repo.

@SaigyoujiYuyuko233
Copy link
Contributor Author

Will take a look at this starting the next week, I do want to ensure a few things:

  • Adding new functionality, should not break previously working stuff (meaning people should be able to opt-in not opt-out of additional config setup) - this also prevents default config errors.
  • Another consideration is to not build a monolith, as I think a 3rd party plugin can/should go only so far, given how just one change can render previously usable things unusable.
  • Easily maintainable (more so, for this being a hobby project).

None of these are specific to any code you have written, as I haven't found time to review code so far. I just want to make sure it goes by the same general rules of the repo.

For the first rule, I can't really test that because v0.1.0-1 does not work for me since I'm using headscale & non-deck operator. But I think people can upgrade it without extra steps because I config default value for them.
For the second & third, sure, keep it simple and stupid

because that is not its job and adding checker will increase the complexity
@saumya-banthia
Copy link
Owner

For the first rule, I can't really test that because v0.1.0-1 does not work for me since I'm using headscale & non-deck operator.

I tested the code with a non-headscale setup. Needed a few changes, will be pushing it to a temp PR branch, test it with your setup and report back. I'll update this comment with the PR branch link, once it is pushed.

@saumya-banthia
Copy link
Owner

Pushed https://github.com/saumya-banthia/tailscale-control/tree/pr12

@SaigyoujiYuyuko233
Copy link
Contributor Author

SaigyoujiYuyuko233 commented Feb 4, 2024

Pushed https://github.com/saumya-banthia/tailscale-control/tree/pr12

Should I mark this pr as resolved for you and you can create a new one?

@saumya-banthia
Copy link
Owner

saumya-banthia commented Feb 4, 2024

Pushed https://github.com/saumya-banthia/tailscale-control/tree/pr12

Should I mark this pr as resolved for you and you can create a new one?

Does the pr12 branch work for you (I can only confirm the non-headscale version to be working)?

@SaigyoujiYuyuko233
Copy link
Contributor Author

SaigyoujiYuyuko233 commented Feb 4, 2024

Pushed https://github.com/saumya-banthia/tailscale-control/tree/pr12

Should I mark this pr as resolved for you and you can create a new one?

Does the pr12 branch work for you (I can only confirm the non-headscale version to be working)?

Not sure if this is a special case, I can't type into the input box.

This work around works for me:

  1. Delete
    let login_server: string, custom_flags: string = '';
  2. Put the following lines into Popup function (does not match the picture below, but you get it)
const [login_server, set_login_server] = useState<string>(tailscaleLoginServer);
const [custom_flags, set_custom_flags] = useState<string>(tailscaleUpCustomFlags);
  1. Replace onChange field from TextFields
  2. Replace value field from TextFields

BTW, moving L228(same as above) into Popup function and Replace value field from TextFields will not work.

It will look like this:
image

Edit:
I guess all the input option inside a modal popup should have its local variable inside the function, because setting exit node ip is not working either.
Another hypothesis is that the issue is cause by tailscale restart operation.
image
After tailscale goes down, it will not turn back on because the device list get wiped which cause by the error above.
Just an inital observation, might not be the cause

@saumya-banthia
Copy link
Owner

I'll look into this, seems a recent DFL or Decky update might have caused it. On a side note: I faced a personal loss a few days back, so might take a bit longer to review things, expect delay in response.

@saumya-banthia
Copy link
Owner

@SaigyoujiYuyuko233: Pushed few more changes to tree#pr12, check to see if this works with your setup.

@ticky: pushed a fix to #14

@SaigyoujiYuyuko233
Copy link
Contributor Author

@SaigyoujiYuyuko233: Pushed few more changes to tree#pr12, check to see if this works with your setup.

@ticky: pushed a fix to #14

Works beautifully on my deck! Thanks for the nice 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.

2 participants