-
Notifications
You must be signed in to change notification settings - Fork 79
Added Stage 4 for the SaltStack lesson #235
Conversation
* WIP collections stuff Signed-off-by: Matt Oswalt <[email protected]> * use correct branch in collection image URL Signed-off-by: Matt Oswalt <[email protected]> * updating collection examples Signed-off-by: Matt Oswalt <[email protected]> * Add cumulus Signed-off-by: Matt Oswalt <[email protected]> * formatting changes Signed-off-by: Matt Oswalt <[email protected]> * Added lessons to juniper curriculum Signed-off-by: Matt Oswalt <[email protected]> * shorten fields Signed-off-by: Matt Oswalt <[email protected]> * Add pp Signed-off-by: Matt Oswalt <[email protected]> * Last-minute updates to changelog and collections meta Signed-off-by: Matt Oswalt <[email protected]> * update paths with new lesson directory mount point Signed-off-by: Matt Oswalt <[email protected]> * update changelog Signed-off-by: Matt Oswalt <[email protected]> * Modified Dockerfile for Salt image and added guide.md for stage 4
o# the commit. Syncing with the master repo
Signed-off-by: skondvilkar <[email protected]>
Since there was some flux earlier today, can you let me know when this is ready for review? You can feel free to work on it in the meantime if not, just mark it as "draft". Let us know either way so I or @cloudtoad can take a look at it. |
@Mierdin it is ready for review. These are our final changes for stage 4. Sorry for the confusion earlier. |
Please open a new PR to submit a changelog update
…On Mon, Jul 8, 2019, 5:27 PM Derick Winkworth ***@***.***> wrote:
***@***.**** approved this pull request.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#235?email_source=notifications&email_token=ABAIZ633FST7PMOAPHLMRW3P6OWKLA5CNFSM4HZELCXKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB5ZI5VI#pullrequestreview-259165909>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABAIZ66S6I6TTSXGBBMQ2TLP6OWKLANCNFSM4HZELCXA>
.
|
Signed-off-by: skondvilkar <[email protected]>
updated changelog.md
changelog file updated |
Thanks. Ignore my comment about a "new PR". For some reason I thought this was merged. Putting the changelog update in this PR is the right action. |
I will check this branch this evening. out and update it so that we can merge it with the current version of the antidote platform. |
Thanks, @cloudtoad. Let me know if there's anything I can help with. |
It sounds like this should be part of v1.0.1 so I added it to that project plan. |
@skondvilkar My sincere apologies for taking so long on this. We're kickstarting the next curriculum release to coincide with the relaunch activities that are taking place in about a month, so we are looking to do a release within the next two weeks. As part of this, I'll be looking to merge this work (finally). One thing I wanted to ask is, since it's been so long, if I review this, are you able to make minor fixes as I bring them up within that timeframe? If not that's fine, just trying to plan how I spend my time in the next few weeks. Thanks again for this. |
@Mierdin sure. Let me know what changes will be required, and I'll make them accordingly. It's been a while since I worked with the NRE labs platform. |
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 general, this is great. Just some minor things I'd like to see adjusted, mostly to reduce long-term complexity. The content itself is awesome.
Thanks!
@@ -25,6 +25,16 @@ COPY ./salt_configs/vqfx1.sls /srv/pillar | |||
# Add pillar file for top | |||
COPY ./salt_configs/top.sls /srv/pillar | |||
|
|||
# Add salt file for infrastructure data |
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 general, we like to avoid loading images up with lesson-specific configuration files and other scripts. Any reason why these files couldn't go in the root of the lesson directory, or perhaps stage4 directory?
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.
Looking more broadly at the Dockerfile, looks like this was already being done. Since you've agreed to let me finish this out, I'll just focus on moving these three files out of the salt image and into the stage4 directory (and will accordingly update the stage 4 lesson guide). However, all other per-stage files should be kept out of the image to keep it more reusable, so I opened #295 to follow up on this in a separate PR later.
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.
Eh.....after thinking about it a bit more, the lesson works well enough for now, so I'm going to leave things the way they are. In #295, we'll work to move these files out of the image and into perhaps the configs directory where they can be placed in the correct location on behalf of the user, without muddying the image for others that might want to use it. For now, nevermind. :)
``` | ||
cat /srv/pillar/infrastructure_data.sls | ||
``` | ||
<button type="button" class="btn btn-primary btn-sm" onclick="runSnippetInTab('salt1', 0)">Verify Output (Optional)</button> |
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.
The snippet index you're providing here is actually not needed anymore. You can provide the this
keyword and antidote-web will take care of identifying the correct snippet. You can still use this old way of doing things but it's more brittle, so I'd prefer you update these. See below
<button type="button" class="btn btn-primary btn-sm" onclick="runSnippetInTab('salt1', 0)">Verify Output (Optional)</button> | |
<button type="button" class="btn btn-primary btn-sm" onclick="runSnippetInTab('salt1', this)">Verify Output (Optional)</button> |
@skondvilkar Any updates? We're gonna be looking to do a curriculum release pretty soon. We can either delay this PR until the next release, or if you're okay with it, I can make the changes I'm requested above on your behalf. Just let me know |
@Mierdin, if you could make the changes, that would be great. I have some last-minute deadlines to meet, and I'll also have to spin up the environment on my machine. Sorry for the delay. |
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
…d remain open to additional stages in the future Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
I added a few commits, all of which were aimed at fixing pretty minor issues. In my testing, everything seems to work properly, so I'm going to merge this now. Please let me know ASAP if you think anything else should be changed - if it's a small change, we might be able to get it in before v1.1.0 is released sometime next week. Otherwise, it will have to wait until later. Thanks again for adding this stage to the lesson! |
@Mierdin, this looks good to me. Thank you for making the changes. |
No problem - thank you for verifying |
Added Stage 4 with the following addition or modification of the files: