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

update ServiceProfile compat warning #1661

Merged
merged 3 commits into from
Aug 21, 2023
Merged

update ServiceProfile compat warning #1661

merged 3 commits into from
Aug 21, 2023

Conversation

hawkw
Copy link
Contributor

@hawkw hawkw commented Aug 21, 2023

This commit updates the HTTPRoute/ServiceProfile incompatibility warning in the documentation to use the new wording suggested by @wmorgan in #1657 (comment).

This commit updates the HTTPRoute/ServiceProfile incompatibility warning
in the documentation to use the new wording suggested by @wmorgan in
#1657 (comment).
Copy link
Member

@kflynn kflynn left a comment

Choose a reason for hiding this comment

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

Hmmm. I'm seeing places in the Netlify preview where e.g. [ServiceProfile]s isn't rendered as a link. I like the content, though! 🙂

[ServiceProfile](../../features/service-profiles/) is also defined for that
Service, proxies will use the ServiceProfile configuration, rather than the
HTTPRoute configuration, as long as the ServiceProfile exists.
**Outbound HTTPRoutes and [ServiceProfile]s provide overlapping configuration.**
Copy link
Member

Choose a reason for hiding this comment

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

Huh. This is rendering as

Outbound HTTPRoutes and [ServiceProfile]s provide...

in the Netlify preview? This is weird to me because I thought I checked the preview for your other PR, but... yeah, maybe doublecheck that? 🤔 🙁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gah, whoops --- i think i forgot to add a link reference in this one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

turns out this is the same problem as #1662 (comment) 🙃

@hawkw hawkw requested a review from kflynn August 21, 2023 18:47
Copy link
Member

@kflynn kflynn left a comment

Choose a reason for hiding this comment

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

Looks good, ship it! 🙂

@hawkw hawkw merged commit 85a0389 into main Aug 21, 2023
7 checks passed
@hawkw hawkw deleted the eliza/update-warning branch August 21, 2023 18:53
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