-
Notifications
You must be signed in to change notification settings - Fork 212
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
base: main
Are you sure you want to change the base?
Conversation
f73345d
to
9e6f671
Compare
There was a problem hiding this 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 togreengrass
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 thegreengrass
root samples folder to link to individual sampleREADME.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 thegreengrass/README.md
which platforms are supported to prevent customers who try to run the sample from being confused when following provided steps.
@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. |
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. |
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.