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 custom public address in generated configurations #8

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

guzalv
Copy link
Contributor

@guzalv guzalv commented Jan 2, 2025

Fixes #1

@guzalv
Copy link
Contributor Author

guzalv commented Jan 2, 2025

I started with adding URL parameters for public IP/port, and I have my first questions

Should we add a parameter for each of IP and port, or 1 for the whole address, including IP and port? Having 2 parameters is simpler (no need for string manipulation, edge cases). Having just 1 seems simpler from the user's perspective.

A related question is whether it makes sense to customize only one of address and port. Perhaps even if we use 2 different parameters, it should be enforced that both are present?

Update: removed port handling, I realized it's unrelated from this feature.

@coveralls
Copy link

coveralls commented Jan 2, 2025

Pull Request Test Coverage Report for Build 12931715286

Details

  • 0 of 4 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.4%) to 9.677%

Changes Missing Coverage Covered Lines Changed/Added Lines %
main.go 0 4 0.0%
Totals Coverage Status
Change from base Build 12599671838: -0.4%
Covered Lines: 9
Relevant Lines: 93

💛 - Coveralls

@guzalv guzalv force-pushed the support-custom-public-address branch 6 times, most recently from fd3e225 to 6b8cded Compare January 3, 2025 14:43
@guzalv guzalv force-pushed the support-custom-public-address branch 2 times, most recently from e11d557 to e2f6e28 Compare January 19, 2025 16:31
Copy link
Member

@rg0now rg0now left a comment

Choose a reason for hiding this comment

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

We're almost there, only minor stuff missing. Once ready please try to repack the whole thing into a single commit so that we can simply git-merge and still get a clean history. Thx!!!

@guzalv guzalv force-pushed the support-custom-public-address branch from dd50b2c to df0d645 Compare January 22, 2025 13:00
@guzalv
Copy link
Contributor Author

guzalv commented Jan 22, 2025

We're almost there, only minor stuff missing. Once ready please try to repack the whole thing into a single commit so that we can simply git-merge and still get a clean history. Thx!!!

I have now made fixes and squashed all commits into one. Thanks for your comments!

@guzalv guzalv marked this pull request as ready for review January 22, 2025 13:02
Copy link
Member

@rg0now rg0now left a comment

Choose a reason for hiding this comment

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

Sorry for the nitpicking again. I promise this is the last time. Let me know if you're fed up with this and want me to jump in and to the renamings all over the place

@guzalv guzalv force-pushed the support-custom-public-address branch from df0d645 to e50d49a Compare January 23, 2025 14:36
@guzalv guzalv force-pushed the support-custom-public-address branch from e50d49a to a53b400 Compare January 23, 2025 14:48
@guzalv
Copy link
Contributor Author

guzalv commented Jan 23, 2025

Thanks, and no worries at all! I should have done this by myself after you recommended to change the env variable. I have made the fixes, let's see how it looks now and still correct what's necessary.

@rg0now rg0now merged commit 460af1e into l7mp:main Jan 23, 2025
3 checks passed
@rg0now
Copy link
Member

rg0now commented Jan 23, 2025

Thanks a lot, applied!

@guzalv guzalv deleted the support-custom-public-address branch January 24, 2025 11:52
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.

Incorrect public IP
3 participants