search

githubtraining / continuous-delivery-aws

This repository is the source for the Learning Lab course, GitHub Actions: Continuous Delivery
https://lab.github.com/githubtraining/github-actions:-continuous-delivery
Creative Commons Attribution 4.0 International
3 stars 12 forks source link

Clarification needed in step 1 #6

Closed bdougie closed 4 years ago

bdougie commented 4 years ago

Hello, I am running through this because this may be interesting content to share GitHub Universe next week in the Actions Hackspace.

My comments below are a direct correlation with this Workspace in mind.,

Is the content complete?

I ask because while evaluating it, I am finding the steps a bit confusing.

I am also presented with multiple branches when I start. I have not seen another lab course start like this.

Screenshot 2019-11-07 13 34 03

Confusion

The instructions state I should edit the staging-workflow.yml file, but only the following files are include in the CHANGETHIS folder

Changing the deploy-staging.yml works, but multiple files threw me off at the happy path.

Suggestion

  1. I can open a PR to fix the content, but I might suggest also using quicklinks similar to how the hello world action lab works. That experience seemed really nice and I would love to see that same experience here.

  2. I can open a separate PR for that to start the discussion. Curious if there was a reason that method wasn't used.

brianamarie commented 4 years ago

Thank you @bdougie! You're right - the file is misnamed, and it would definitely be better to have quicklinks to edit the file.

In terms of the extra branches, this is something we battle with Learning Lab. New branches are the only way we can introduce new commits, and we have to choose between opening the PRs all at the beginning, or having the branches show up as you're seeing them.

It would be 💯 to have these things fixed before Universe, since this will also be used in the workshops. If you have the bandwidth to open a PR that would be amazing - but if not, I will see what our team's bandwidth is to work on this. (cc @mattdavis0351 - would this be something you could focus on on Tuesday?)

bdougie commented 4 years ago

I have time to dedicate to this later today and tomorrow. Happy to help.

mattdavis0351 commented 4 years ago

@brianamarie @bdougie I have created a PR #10 that addresses this file change. It actually needs to be changed in the responses as well as in the config.

@brianamarie @hectorsector if I could get a review on it we can merge and close this issues as well as the associated PR's

brianamarie commented 4 years ago

Thank you @bdougie! This has been addressed through your work, #10, and #12. 🎉