Skip to content

feat: promote ingress settings to public #177

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Dec 7, 2021
Merged

feat: promote ingress settings to public #177

merged 3 commits into from
Dec 7, 2021

Conversation

@jawnsy
Copy link

@jawnsy jawnsy commented Dec 6, 2021 •
edited
Loading

  • Add ingress section to values.yaml and re-generate documentation
  • Add tests for ingress settings
  • Change comment syntax to {{/* */}} to prevent it from appearing in the generated Ingress spec

@shortcut-integration
Copy link

shortcut-integration bot commented Dec 6, 2021

This pull request has been linked to Shortcut Story #18924: Remove compatibility code for old ingress controller.

@jawnsy jawnsy self-assigned this Dec 6, 2021
@jawnsy jawnsy marked this pull request as ready for review December 6, 2021 23:50
@jawnsy jawnsy requested a review from misskniss December 6, 2021 23:55
@jawnsy
Copy link
Author

jawnsy commented Dec 7, 2021

This has some of the changes in #169 (namely, exposing the ingress in our docs), but I split it into a separate PR to make it less risky and easier to review

feat: promote ingress settings to public
60a8175 
* Add ingress section to values.yaml and re-generate documentation
* Add tests for ingress settings
* Change comment syntax to {{/* */}} to prevent it from appearing
  in the generated Ingress spec
@jawnsy jawnsy force-pushed the jawnsy/sc-18924/add-ingress-public branch from 07913a2 to 60a8175 Compare December 7, 2021 00:05
@jawnsy jawnsy requested a review from Emyrk December 7, 2021 00:06
johnstcn
johnstcn approved these changes Dec 7, 2021
View reviewed changes
Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

I haven't tested this, but it looks fine to me. Some minor comments.

Emyrk
Emyrk reviewed Dec 7, 2021
View reviewed changes
Copy link
Member

@Emyrk Emyrk left a comment

Choose a reason for hiding this comment

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

Those tests look tedious to write!

@jawnsy
Copy link
Author

jawnsy commented Dec 7, 2021

Those tests look tedious to write!
@Emyrk They are kinda tedious, yeah 😦 But less tedious than always testing things manually, I think!

Do you have ideas for how to improve/simplify the API or test code? I'm open to suggestions!

I'm not in love with this code, but it's better than the alternative of manual tests only

@jawnsy
Copy link
Author

jawnsy commented Dec 7, 2021

Tested this in dev-4 and everything deploys fine, should be safe to merge

@jawnsy jawnsy merged commit 92e8f3b into main Dec 7, 2021
@jawnsy jawnsy deleted the jawnsy/sc-18924/add-ingress-public branch December 7, 2021 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@johnstcn johnstcn johnstcn approved these changes

@Emyrk Emyrk Emyrk left review comments

@coadler coadler Awaiting requested review from coadler

@ericpaulsen ericpaulsen Awaiting requested review from ericpaulsen

@mterhar mterhar Awaiting requested review from mterhar

@misskniss misskniss Awaiting requested review from misskniss

Assignees

@jawnsy jawnsy

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jawnsy @johnstcn @Emyrk