-
Notifications
You must be signed in to change notification settings - Fork 79
New Lesson - Junos Automation with PyEZ #117
Conversation
Is this ready for review? |
Yes, it's ready for review. Apologize for not mention that. Thanks! |
No worries. FFR, there are labels you can use for WIP or RFR as shorthand
…On Wed, Oct 31, 2018, 1:05 AM Raymond Lam ***@***.*** wrote:
Yes, it's ready for review. Apologize for not mention that. Thanks!
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<https://github.com/nre-learning/antidote/pull/117#issuecomment-434595267>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AECM-_edrucDCmV2ElXzLDkLt1SabnoRks5uqVltgaJpZM4X_t0j>
.
|
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.
@jnpr-raylam This is an EPIC lesson. Thanks so much for working on this, it's clear you spent a lot of time on it.
My biggest concern is that this is a TON of content, which is a good problem to have, but we need to make sure we keep a few things in mind to preserve the spirit of the site:
- Don't gloss over explanations in favor of interactive examples. It's good to have the code snippets (and boy do you have a lot of them) but we need to make sure the user is mentally along for the ride, not just clicking the buttons
- The examples should flow a little better. In many cases, it's not clear if you intended something to work or not work - if you are having them run a command but it's not meant to work, highlighting this with a comment is useful. There are several places where you have a set of snippets for them to run, with no paragraphs in between. It's those breaks between examples where we get an opportunity to keep them hooked with explanations, so let's make sure to do that.
- I also noticed in a few places you started a section with a huge code snippet they don't have to run, then you repeat parts of that snippet as interactive examples. Rather than the large snippet up front, take a moment to explain what they're about to do in plain-english. Then they'll want to understand the smaller snippets more readily.
- This is a lot of content (again, good problem) and it can get tough to get through it all. Some ideas: maybe break up into even more stages (i.e. stage 2 is huge)? Also, adding images can help break up the monotony
Thanks again for this great lesson; I am excited to get this merged soon!
lessons/lesson-24/syringe.yaml
Outdated
--- | ||
lessonName: Junos Automation with PyEZ | ||
lessonID: 24 | ||
category: introductory |
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.
I would definitely put this in the configuration
category
lessons/lesson-24/stage1/guide.md
Outdated
For HTTP, telnet to web server port 80 | ||
|
||
``` | ||
telnet labs.networkreliability.engineering 80 |
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.
Not sure how well documented this is (will go make sure of this now) but no internet access is permitted outside the cluster - and this includes even the external IP address this will resolve to. My suggestion is to either:
- Spin up your own webserver
- Skip this example and move right into SSH/NETCONF.
My preference would be the latter, since the REST APIs lesson does some basic curl
examples, and this isn't as closely related to this lesson's content (netconf)
lessons/lesson-24/stage1/guide.md
Outdated
|
||
To get the web page, issue the GET command, following the relative path of the web page, and then specify the HTTP version, and at last provide the 'Host' field to specify the web host. | ||
|
||
<pre> |
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.
Again I would prefer skipping this example but if you keep it in, please use the three-backticks model consistently for snippets.
lessons/lesson-24/stage1/guide.md
Outdated
``` | ||
<button type="button" class="btn btn-primary btn-sm" onclick="runSnippetInTab('linux1', 3)">Run this snippet</button> | ||
|
||
#### 3. What is PyEZ |
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 section reads a bit rough. Note that folks will be likely going through this step by step, and in this section, they'll be overwhelmed with a bunch of list items. Paragraphs with illustrative examples will win here.
lessons/lesson-24/stage1/guide.md
Outdated
- perform regular expression matching on the response | ||
- extract the information | ||
- a litter bit easier compared to pure Python code | ||
- <a href="https://raw.githubusercontent.com/jnpr-raylam/antidote/lesson-24/lessons/lesson-24/stage1/sample_code/get-uptime-ncclient.py" target="_blank">Sample code here: get-uptime-ncclient.py</a> |
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.
In these links, please use the main nre-learning/antidote
repo
lessons/lesson-24/stage2/guide.md
Outdated
|
||
``` | ||
print(dev.display_xml_rpc('show interfaces em3', format='text')) | ||
intf = dev.rpc.get_interface_information(interface_name='em3') |
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.
Nothing is done with many of these intf
variables.
@@ -0,0 +1,123 @@ | |||
## Junos Automation with PyEZ | |||
|
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.
Please give yourself credit near the top of each guide. Something like **Contributed by @jnpr-raylam**
lessons/lesson-24/stage2/guide.md
Outdated
Unsupported `OR` operator: | ||
|
||
``` | ||
xml.findall('ifd[mtu="1514" or ifl]') |
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.
On this and the next two examples, I got SyntaxError: Invalid predicate
lessons/lesson-24/stage2/guide.md
Outdated
<pre> | ||
for ifd in intf.xpath('physical-interface'): | ||
name = ifd.xpath('name')[0].text | ||
mac = ifd.xpath('hardware-physical-address')[0].text |
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.
I'd recommend adding a comment or some other notice that makes it clear that this should fail
|
||
For this syntax, call the `load()` and `commit()` function of `cu` variable to load and commit config on Junos device | ||
|
||
``` |
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.
Starting here, the rest of the examples in this section don't have "run this snippet" buttons. Is this intentional?
### Junos Automation with PyEZ | ||
|
||
#### Part 4 - PyEZ Tables and Views | ||
|
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.
A brief introduction to what these are, as well as why someone should care about them, would be a good idea.
@Mierdin, Thanks for your comment, I have updated the lesson accordingly, could you please review again. FYI, in some code snippets, I intentionally use |
I'm still of the opinion that a few of the sections are extremely long, bordering on too exhaustive. It's great detail, but the spirit of the content shouldn't be as a reference - we have docs for that. We need to focus on illustrative examples, where we get the maximum bang for our buck. You've made great strides towards this in this PR but I think there's more that can be done here. Could you consider breaking up stage 3? It's still clocking in at 557 lines right now. Also, across the entire lesson, consider areas where you could be a little more terse. Some of the examples are definitely more compelling than others, and they tend to get drowned in the noise a little bit. Remember that the goal is for someone to be able to get through a lesson stage in under 5 minutes. Hopefully that's not too generic, and also hopefully you're not getting the sense that I'm asking you to throw out huge chunks of what you've worked on - I'm not. I just want to make sure that what content is there has the maximum impact on an audience, that likely hasn't seen any pyez or python before. |
@Mierdin, Thanks for your comment, I have trimmed down the lesson content to make to more clear and suitable for beginners. Hope that matches the NRE lessons' spirit. |
@jnpr-raylam Thanks - at first glance it's looking a lot better. Will look in more detail shortly, but in the meantime, why remove the tables and views lab? It's a useful component of pyez and think it should be included - my main concern was the length and verbosity on some of the middle stages - the tables and views one was solid as-is, mostly. Esp considering it's on a separate stage, I'd love to see it re-included unless you feel strongly otherwise. |
@Mierdin, Originally I thought the content is too much, but thanks for your recommendation, I added back the PyEZ Tables section and trimmed down to remove advanced Tables topic. |
Thanks. Yeah the number of stages is fine. I'd rather have more stages that are short enough to get through quickly than fewer stages that might take more time than 5-7 minutes |
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.
LGTM - thanks again!
stage1: Introduction
stage2: Collect and Parse Information from Junos Devices
stage3: Configuration Management
stage4: PyEZ Tables and Views