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

Mandy/updated device sdk guide #886

Merged
merged 11 commits into from
Aug 12, 2024
Merged

Conversation

DevMandy
Copy link
Contributor

@DevMandy DevMandy commented Aug 8, 2024

This is a new PR that replaces this one. I pulled out the changes in scope and will handle the additional requests around marking steps as optional in a separate PR.

Copy link

vercel bot commented Aug 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
ngrok-docs ✅ Ready (Inspect) Visit Preview Aug 12, 2024 11:13pm

@DevMandy
Copy link
Contributor Author

DevMandy commented Aug 8, 2024

I'm working out permissions to push the agent code to this repo. The code resides in this repo now but is missing requirements.txt and some updates to the readme. I am pushing a new repo so I don't need to wait for Shub to return from vacation to give me permission to push to the initial repo. Additionally, I'd like to rename this repo to include Python in the name, so this seemed like a good solution.

Copy link
Contributor

@joelhans joelhans left a comment

Choose a reason for hiding this comment

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

Hey! A few small thoughts throughout and a bigger question for us to hash on with @stmcallister.

I think we should just move forward with your new repo (it does indeed have a better name) and we can archive or delete the old one whenever you're ready.

This guide provides step-by-step instructions for using ngrok as a device gateway.
This example shows you how to embed connectivity into an application using ngrok's Python SDK to access
an API running on an IoT device. All the Python code in this example is available in
[this repo](https://github.com/ngrok-samples/ngrok-python-iot-agent).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[this repo](https://github.com/ngrok-samples/ngrok-python-iot-agent).
[the `ngrok-samples/ngrok-python-iot-agent` repo](https://github.com/ngrok-samples/ngrok-python-iot-agent).

I like being a bit more explicit with the link target, but take it or leave it!

Copy link
Contributor

Choose a reason for hiding this comment

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

Somewhere along my journey, when submitting a PR for some docs, I was taught the same thing. I like the verbose link titles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! This is very helpful feedback. I didn't consider that.


Next, add two `CNAME` records to your DNS provider's registry, providing values from the previous response.

First add a CNAME record for the wildcard subdomain you just created.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
First add a CNAME record for the wildcard subdomain you just created.
Add a CNAME record for the wildcard subdomain you just created.


## Create a bot user

First, you’ll create a bot user so that in the next section, you can create an agent authtoken independent of any user account.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
First, you’ll create a bot user so that in the next section, you can create an agent authtoken independent of any user account.
Create a bot user so that in the next section, you can create an agent authtoken independent of any user account.

docs/guides/device-gateway/sdk.md Outdated Show resolved Hide resolved
docs/guides/device-gateway/sdk.md Outdated Show resolved Hide resolved
docs/guides/device-gateway/sdk.md Show resolved Hide resolved
docs/guides/device-gateway/sdk.md Show resolved Hide resolved
with the value you set on line `109` of the code and `{URL_PART}` with the value of the
`url_part` property of the list tunnels request:

```curl
Copy link
Contributor

Choose a reason for hiding this comment

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

Last one!

docs/guides/device-gateway/sdk.md Outdated Show resolved Hide resolved
on `device342.sitea.{YOUR_DOMAIN}` and `device896.sitea.{YOUR_DOMAIN}` without having to reserve them individually.
It can be helpful to create a separate subdomain for each site you wish to connect to.

Run the following command, substituting your API key for `{API_KEY}` and your domain for `{YOUR_DOMAIN}`:
Copy link
Contributor

Choose a reason for hiding this comment

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

@DevMandy @stmcallister (and honestly everyone else who writes docs...)

We should come up with a standard way of defining these "variables" in code blocks that users need to find and replace. I don't have a strong opinion about which one, but I think if we're consistent internally then they'll more easily be able to pick them out of complex code/config.

Any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Completely agree! I ❤️ consistency! I would vote to find out if we already use a particular standard more often and stick to that one. I don't have strong feelings about which style of syntax we use just that it's consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment, these are consistent across guides (or at least new guides). I can look at making them consistent as part of the guides reboot project. I agree they should be consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was using <API_KEY> in previous guides, but I'm happy to follow your path here with the brackets. They do stick out nicely from typical YAML/JSON syntax.

Copy link
Contributor

@stmcallister stmcallister left a comment

Choose a reason for hiding this comment

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

Overall, looks great! I agree with Joel's suggestions. Plus, I added a couple of my own.

This guide provides step-by-step instructions for using ngrok as a device gateway.
This example shows you how to embed connectivity into an application using ngrok's Python SDK to access
an API running on an IoT device. All the Python code in this example is available in
[this repo](https://github.com/ngrok-samples/ngrok-python-iot-agent).
Copy link
Contributor

Choose a reason for hiding this comment

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

Somewhere along my journey, when submitting a PR for some docs, I was taught the same thing. I like the verbose link titles.

https://api.ngrok.com/bot_users
```

You should receive a 201 response similar to the following:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
You should receive a 201 response similar to the following:
You should receive a `201` response similar to the following:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NO ONE catches stuff like this! I made that mistake in the last 5 guides and no one caught it. Thank you! :)

docs/guides/device-gateway/sdk.md Outdated Show resolved Hide resolved
}
}
```

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it me, or did that indentation get crazy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it was wonky! Thank you!

Copy link
Contributor

@stmcallister stmcallister left a comment

Choose a reason for hiding this comment

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

LGTM! 👍 🎉 🌮

@DevMandy DevMandy merged commit 3fc99cc into main Aug 12, 2024
3 checks passed
@DevMandy DevMandy deleted the mandy/updated-device-sdk-guide branch August 12, 2024 23:33
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