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

docs: adding documentation for paralus addon #714

Merged
merged 5 commits into from
Jun 22, 2023

Conversation

niravparikh05
Copy link
Contributor

Issue #, if available:

Description of changes:

Adding documentation for paralus addon which got merged as part of aws-samples/cdk-eks-blueprints-patterns#67

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

parkand1
parkand1 previously approved these changes Jun 6, 2023
Copy link
Contributor

@parkand1 parkand1 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@shapirov103 shapirov103 left a comment

Choose a reason for hiding this comment

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

@niravparikh05 @parkand1
If you read this doc, it is very confusing what pattern we are talking about, since the pattern is in another repo. My suggestion is this:

  1. Change this PR and only provide a general description of the add-on and a link to the patterns repo with the paralus pattern implementation.
  2. Move all pattern specific narrative away from this PR to a separate PR in the patterns repo.

Link both PRs.

Please use existing OSS and commercial add-ons for reference. For example OSS add-on pixie.

Or DataDog addon.

Just FYI, I am open to add a reference to Rafay in this PR to make it worthwhile their efforts. A small blurb on Rafay with a link to more advanced features for example, will be acceptable.

Copy link
Collaborator

@shapirov103 shapirov103 left a comment

Choose a reason for hiding this comment

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

See my comments

@niravparikh05
Copy link
Contributor Author

@shapirov103 @parkand1 I have restructed the document addressing your comments and feedback, do have a look and let me know if any further changes are required.

Copy link
Collaborator

@shapirov103 shapirov103 left a comment

Choose a reason for hiding this comment

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

Looks better for sure, please see a few comments I left. Happy to discuss.

docs/addons/paralus.md Outdated Show resolved Hide resolved
docs/addons/paralus.md Outdated Show resolved Hide resolved
docs/addons/paralus.md Outdated Show resolved Hide resolved
docs/addons/paralus.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@niravparikh05 niravparikh05 left a comment

Choose a reason for hiding this comment

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

@shapirov103 have addressed your comments, please have a look

docs/addons/paralus.md Outdated Show resolved Hide resolved
docs/addons/paralus.md Outdated Show resolved Hide resolved
docs/addons/paralus.md Outdated Show resolved Hide resolved
docs/addons/paralus.md Outdated Show resolved Hide resolved
Copy link
Contributor

@parkand1 parkand1 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@shapirov103 shapirov103 left a comment

Choose a reason for hiding this comment

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

LGTM

@parkand1 there are some minor editorial changes that we will need to apply, such as rbac = RBAC, punctuation in some places

@shapirov103 shapirov103 merged commit bdf6812 into aws-quickstart:main Jun 22, 2023
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