Skip to content
This repository has been archived by the owner on Dec 3, 2021. It is now read-only.

Added Stage 4 for the SaltStack lesson #235

Merged
merged 14 commits into from
Jan 25, 2020
Merged

Conversation

skondvilkar
Copy link
Contributor

@skondvilkar skondvilkar commented Jun 18, 2019

Added Stage 4 with the following addition or modification of the files:

  1. Added .sls and .conf files under ../nrelabs-curriculum/images/salt/salt_configs
  2. Added guide.md under ../nrelabs-curriculum/lessons/tools/lesson-30-salt/stage4
  3. Modified top.sls file
  4. Modified lesson.meta.yaml
  5. Modified Dockerfile for the Salt image

* 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]>
@Mierdin
Copy link
Member

Mierdin commented Jun 18, 2019

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.

@skondvilkar
Copy link
Contributor Author

skondvilkar commented Jun 18, 2019

@Mierdin it is ready for review. These are our final changes for stage 4. Sorry for the confusion earlier.

@Mierdin Mierdin requested a review from cloudtoad June 18, 2019 23:30
cloudtoad
cloudtoad previously approved these changes Jul 8, 2019
@Mierdin
Copy link
Member

Mierdin commented Jul 8, 2019 via email

@skondvilkar
Copy link
Contributor Author

changelog file updated

@Mierdin
Copy link
Member

Mierdin commented Jul 12, 2019

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.

@cloudtoad
Copy link
Contributor

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.

@skondvilkar
Copy link
Contributor Author

Thanks, @cloudtoad. Let me know if there's anything I can help with.

@Mierdin
Copy link
Member

Mierdin commented Oct 16, 2019

It sounds like this should be part of v1.0.1 so I added it to that project plan.

@Mierdin
Copy link
Member

Mierdin commented Jan 16, 2020

@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.

@skondvilkar
Copy link
Contributor Author

@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.

Copy link
Member

@Mierdin Mierdin left a 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
Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

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>
Copy link
Member

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

Suggested change
<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>

@Mierdin
Copy link
Member

Mierdin commented Jan 24, 2020

@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

@skondvilkar
Copy link
Contributor Author

@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.

@Mierdin
Copy link
Member

Mierdin commented Jan 25, 2020

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 Mierdin merged commit afb3f95 into nre-learning:master Jan 25, 2020
@skondvilkar
Copy link
Contributor Author

@Mierdin, this looks good to me. Thank you for making the changes.

@Mierdin
Copy link
Member

Mierdin commented Jan 27, 2020

No problem - thank you for verifying

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants