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

Feature/kubeshark #1003

Merged
merged 18 commits into from
Jun 12, 2024
Merged

Feature/kubeshark #1003

merged 18 commits into from
Jun 12, 2024

Conversation

zghanem0
Copy link
Contributor

Issue #, if available:

Description of changes:

  • added kubeshark addon
  • updated the index
  • created kubeshark.md for doc

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

Copy link
Collaborator

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

@zghanem0 Firstly thankyou so much for the PR. Very good addon that can be added to blueprints. Before i dive deep in to the PR. Following essential things are missing as part of minimum criteria:

  1. Doc index.md is not updated with your for getting listed in Addons list
  2. mkdocs.yaml is not updated with your new addon for doc website
  3. E2E blueprints example stack is not updated with your addon. Make sure to update the blueprints stack with your addon for E2E and also run the E2E once locally.
  4. Make sure you add new unit tests for your addon. Once done let me know i can review again.

Copy link
Collaborator

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

@zghanem0 Great work, need some minor changes in both PRS, before i can test and merge this.

docs/addons/index.md Outdated Show resolved Hide resolved
mkdocs.yml Outdated Show resolved Hide resolved
docs/addons/kubeshark.md Show resolved Hide resolved
Copy link
Collaborator

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

@zghanem0 One more minor doc update required

docs/addons/kubeshark.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

Please fix this issue asap. This is the reason for GH action failure.

docs/addons/kubeshark.md Show resolved Hide resolved
lib/addons/index.ts Outdated Show resolved Hide resolved
@elamaran11
Copy link
Collaborator

@zghanem0 Please fix the index file removal issue so that fixes GH action error. Once done fixing all issues in both PRs let me know. I will run E2E, review and then can merge. Nice work and thankyou for hanging in there. We have a very rigorous review process. Awesome progress.

@elamaran11
Copy link
Collaborator

Fix the GitHub action errors.

Copy link
Collaborator

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

LGTM. Thankyou for the contribution.

@elamaran11
Copy link
Collaborator

@zghanem0 Only concern at this point for merging this PR is, the addon is currently in your private org, so we want to know who will maintain the Kubeshark addon if its in your private repo, whats your path to move the addon to a public org like aws-samples.

@elamaran11
Copy link
Collaborator

@zghanem0 Based on this comment, merging this. Please followup and update the pattern once the addon is moved to aws-samples.

@elamaran11 elamaran11 merged commit 64d4cc2 into aws-quickstart:main Jun 12, 2024
2 checks passed
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