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

Add ingress messages reference #3535

Merged
merged 9 commits into from
Nov 7, 2024

Conversation

robin-kunzler
Copy link
Contributor

Adds a reference document explaining ingress messaging.

If you are submitting a Pull Request for adding or changing content on the ICP Developer Documentation, please make sure that your contribution meets the following requirements:

@robin-kunzler robin-kunzler marked this pull request as ready for review September 25, 2024 12:43
@robin-kunzler robin-kunzler requested review from a team as code owners September 25, 2024 12:43
Copy link
Member

@oggy-dfin oggy-dfin left a comment

Choose a reason for hiding this comment

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

Looks very good overall, thank you! Left a few small comments and a question, PTAL

docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
Copy link
Contributor Author

@robin-kunzler robin-kunzler left a comment

Choose a reason for hiding this comment

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

Thanks for the reviews and good input @jessiemongeon1 and @oggy-dfin !

Addressed everything, only have two clarifying questions to you Ogi!

docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
Copy link
Contributor

@roelstorms roelstorms left a comment

Choose a reason for hiding this comment

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

Left a few comments. Otherwise LGTM.

@robin-kunzler
Copy link
Contributor Author

Thanks for the additional feedback and input @roelstorms and @yvonneanne ! I addressed everything.

  • @roelstorms since you approved I took the liberty to resolve your comments after addressing them. But if you have anything to add please re-open them!
  • @yvonneanne could you PTAL? If you're happy with the page I'd proceed and merge this :-)

Copy link
Contributor Author

@robin-kunzler robin-kunzler left a comment

Choose a reason for hiding this comment

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

Good catches @yvonneanne , thanks. Addressed your comments, PTAL!

docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
…ngress-messages-reference

# Conflicts:
#	.github/CODEOWNERS
#	sidebars.js
Copy link
Contributor Author

@robin-kunzler robin-kunzler left a comment

Choose a reason for hiding this comment

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

@yvonneanne Thanks again for the additional input. Addressed the comments, could you PTAL?

docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
Copy link
Contributor Author

@robin-kunzler robin-kunzler left a comment

Choose a reason for hiding this comment

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

Thanks for the review and additional feedback @yvonneanne . Addressed everything, PTAL!

docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
docs/references/ingress-messages.mdx Outdated Show resolved Hide resolved
Copy link
Contributor

@yvonneanne yvonneanne left a comment

Choose a reason for hiding this comment

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

Can't find anything else :)

Nice one!

@robin-kunzler robin-kunzler merged commit 20f05c5 into master Nov 7, 2024
4 checks passed
@robin-kunzler robin-kunzler deleted the robin-kunzler/add-ingress-messages-reference branch November 7, 2024 14:18
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.

5 participants