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

feat: add ability to support additional gateway/domain #115

Merged
merged 3 commits into from
Oct 9, 2024

Conversation

joelmccoy
Copy link
Contributor

@joelmccoy joelmccoy commented Oct 7, 2024

Description

Adds ability to override and add an additional gateway to the nginx config.

Related Issue

Relates to #98

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Other (security config, docs update, etc)

Checklist before merging

@joelmccoy joelmccoy requested a review from a team as a code owner October 7, 2024 16:20
catsby
catsby previously approved these changes Oct 7, 2024
Copy link
Contributor

@catsby catsby left a comment

Choose a reason for hiding this comment

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

👍 is this something we have or should have tests to verify the behavior of?

docs/DNS.md Outdated Show resolved Hide resolved
Co-authored-by: Clint <catsby@users.noreply.github.com>
@joelmccoy
Copy link
Contributor Author

👍 is this something we have or should have tests to verify the behavior of?

I'm not sure if we want to explicitly test this based on this comment:
#98 (comment)

but will defer to @mjnagel if anything extra is required here. Happy to clarify in the docs that this isn't tested as well if that helps at all.

@mjnagel
Copy link
Contributor

mjnagel commented Oct 7, 2024

👍 is this something we have or should have tests to verify the behavior of?

I'm not sure if we want to explicitly test this based on this comment: #98 (comment)

but will defer to @mjnagel if anything extra is required here. Happy to clarify in the docs that this isn't tested as well if that helps at all.

@catsby / @joelmccoy definitely feel weird advocating for not testing this, but I think that is what I said in the comment 😆. I may have poorly stated it there - I think if there were an easy way to test this in CI I'd be good with adding testing, but made an assumption that the complexity around a new domain/cert might be non-trivial. I mostly don't want us to invest too much time in a feature that is not required by uds-core and uds packages in general - I see that as the primary place to focus uds-k3d development efforts and testing.

Also worth calling out, as-is I don't think we even have tests of the primary domain nginx pattern (uds.dev). We probably should track the need for a test on the nginx config in general as a future issue. If developing that test naturally made it easy to add a test of the custom domain pattern no issue also handling that.

All that being said - maybe we should more explicitly note which things are "untested" or "experimental" or whatever we want to call them (extra ports, custom domain, coredns overrides).

@catsby
Copy link
Contributor

catsby commented Oct 7, 2024

assumption that the complexity around a new domain/cert might be non-trivial

@mjnagel I was hoping that helm had some kind of easy way to check that "given a template and a config, does helm execute to create render the result we expect" kind of thing, so testing the generated helm result and not so much the final product e.g. (😉) "does it render correctly for multiple domains". If we don't have that, or it's not practical, so be it 👍

@mjnagel
Copy link
Contributor

mjnagel commented Oct 7, 2024

@catsby ah - makes sense, yeah more of unit test type thing rather than an e2e validation. Definitely could add some helm testing with helm template or other tooling to validate.

@catsby catsby self-requested a review October 7, 2024 22:06
@joelmccoy joelmccoy merged commit 4c62e6c into main Oct 9, 2024
3 checks passed
@joelmccoy joelmccoy deleted the add-support-additional-gateway branch October 9, 2024 14:03
justinthelaw added a commit to justinthelaw/uds-k3d that referenced this pull request Oct 9, 2024
…ns#115) (#18)

Co-authored-by: Joel McCoy <joel@defenseunicorns.com>
Co-authored-by: Clint <catsby@users.noreply.github.com>
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.

3 participants