-
Notifications
You must be signed in to change notification settings - Fork 79
Conversation
@Mierdin @cloudtoad We were able to fix the issues listed above. Successfully installed salt and created the master and minion. Was able to Ping and issued commands as well to the minion from the master. now that the lesson with 3 stages are completed, please let us know what the next steps are. Should i be submitting a new pull request? Thanks, |
Nope, this PR will suffice. I will review this and give you some comments. Please bear with me as yours is the first lesson PR so there may be a few wrinkles 😊 |
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 is a really great first PR, in not a long amount of time. Well done!
I have a few comments that I'd like to get addressed before we think about getting this hosted into NRE Labs (which I'm assuming is the ultimate goal) mostly around security. The docker images we want to put into production have to follow a certain standard of security, etc. The majority of my other comments are copyedit stuff.
Again though, really great first pass.
One more note - the build failure is happening because you haven't updated |
@Mierdin Thanks for the feedback Will implement the changes suggested and upload the code to git. |
@sudhiram9 FYI, take a look at https://docs.saltstack.com/en/latest/ref/configuration/nonroot.html - looks like a recent version of Salt does away with the |
@Mierdin Enabled permissions for the user->"antidote" for the directories listed in the link. I was still facing issues. Currently, added the "antidote" user to the sudoer list, as a workaround for this. Will again try the solution given in the link and let you know if i can skip "root" requirement. |
@Mierdin |
@Sudhishna Depends what you mean. Can you elaborate on what you're trying to do? Generally the only configurations you can make are baked into the image, or done as part of the lesson. There's not currently any functionality to configure the linux machines between stages like the network devices. |
Hi @Mierdin , Please verify our changes and also let us know if further changes are required. Thanks, |
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.
Much better! Thanks for the changes. Only a few more, mostly minor changes. I think we'll be ready to merge with one more pass. See comments in-line.
Please also update the CHANGELOG (that's why CI is failing) and edit the description for this PR to summarize the changes made, rather than the old text when you were troubleshooting this lesson.
@Mierdin Hi Matt, made the second level of changes suggested as well. Please verify if everything is okay. |
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.
Great job.
@Mierdin should i be able to verify the lesson via https://ptr.labs.networkreliability.engineering/labs/?lessonId=30&lessonStage=1 cause, when i checked the above link, it wasn't reflecting the changes i made yesterday. Please let me know if i should be checking the lesson elsewhere. Thanks, |
Please keep these PRs focused on the content being changed - save operational discussions for Slack. |
Changes 1:
Changes 2: