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 Greengrass V2 IPC samples using new IPC-Client V2 #487

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Kriechi
Copy link
Member

@Kriechi Kriechi commented Jul 21, 2023

Description of changes:

This PR adds new samples for Greengrass using the IPC Client V2.
See the included readme at samples/greengrass_v2/README.md for more details.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@sbSteveK sbSteveK left a comment

Choose a reason for hiding this comment

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

This looks solid. Requesting some changes but they're up for debate if there's any disagreement.

  • rename the greengrass_v2 folder to greengrass and restore the deleted sample and move it into that folder along with its .md readme file. Comments can be added that direct customers to v2 but since v1 is still here and may be in use, I'm not sure we want to remove existing resources yet.

  • Update the samples/README.md to reflect moved old sample and include links to the readme to newly added greengrass v2 samples.

  • change the README.md in the greengrass root samples folder to link to individual sample README.md files in each individual sample directory with instructions per sample written similarly to how the other samples have instructions on what they do and how to run. Shared setup/info can be placed in the root readme but it should be clear what is required and how to use each individual sample and any omitted setup information should be linked back to for ease of use.

  • rename the code.py files to reflect the contents of the sample for clarity. The naming stucture of other samples should be used as a style guide. e.g. "greengrass_pubsub.py"

  • Regarding recipes, our SDK supports Linux, MacOS, and Windows. If these components are expected to support the same, please rewrite them to include all platforms supported instead of exclusively linux or provide instructions in generating recipes the way the current ipc_greengrass.md does. If they do not support all three platforms, please explicitly include in the greengrass/README.md which platforms are supported to prevent customers who try to run the sample from being confused when following provided steps.

@Kriechi
Copy link
Member Author

Kriechi commented Aug 23, 2023

@sbSteveK to the first and second point: I did not delete any samples for GGv1 -- the file that shows up in the diff is already a GGv2 IPC sample, and I only renamed + moved it. Or do you mean a different one?

I'll take a look at your other points.

@jmklix jmklix added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 2 days. and removed needs-review labels Sep 1, 2023
@github-actions
Copy link

github-actions bot commented Sep 1, 2023

Greetings! It looks like this PR hasn’t been active in longer than a week, add a comment or an upvote to prevent automatic closure, or if the issue is already closed, please feel free to open a new one.

@github-actions github-actions bot added closing-soon This issue will automatically close in 5 days unless further comments are made. closed-for-staleness and removed closing-soon This issue will automatically close in 5 days unless further comments are made. labels Sep 1, 2023
@github-actions github-actions bot closed this Sep 6, 2023
@bretambrose bretambrose reopened this Sep 6, 2023
@bretambrose bretambrose added automation-exempt This issue will not be closed by autoclose action and removed closed-for-staleness labels Sep 6, 2023
@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 2 days. label Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation-exempt This issue will not be closed by autoclose action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants