Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI: Add link checker & refactor two files into one #266

Merged
merged 73 commits into from
Aug 11, 2020

Conversation

mrjones-plip
Copy link
Contributor

@mrjones-plip mrjones-plip commented Aug 4, 2020

See #259

This PR does a number of things:

  • Decides on Muffet as the utility to check links
  • Adds a bash script muffet.sh to normalize how we run muffet both in GH Actions and for local dev edits
  • Merges the now defunct build-any.branch.yml into ci.yml by using if statements in the ci.yml file. This allows us to not have the complexity of muffet et al. in multiple locations
  • Refactors the ci.yml to have a more standardized format with each step having a name, no longer isolated run calls and some other niceties
  • Adds steps for devs to run muffet locally (tl;dr install go, install muffet, run hugo server, run muffet script)

For QA/Code reivew

  • Ensure that the new muffet.sh script runs locally with out issue. This includes not getting 429 errors (too many requests) from github and running in under ~120 seconds.
  • Ensure that CI scripts will run in the correct scenarios:
    • all branches: branch should pass hugo server and pass muffet.sh. it should not push live to GH pages
    • master: should do all of the above, but do a hugo --minify build call and then push live to GH Pages docs site

I've done a bunch of testing (say hello to my bazillion debug/test commits below) such that I think this is fine to just merge to master (after QA/review of course), but I'm open to more testing techniques. I suspect the worst scenario is that it pushes to master, thus live, when we don't want, so pay special attention to this stanzy in the ci.yml:

- name: Master Branch Only - Deploy to GH pages
  if: ${{ github.ref == 'refs/heads/master' }}
  uses: peaceiris/actions-gh-pages@v3
  with:
    personal_token: ${{ secrets.DEPOLY_TO_SITE}}
    external_repository: medic/medic.github.io
    publish_dir: ./public
    user_name: medic-ci
    user_email: medic-ci@github
    publish_branch: master

@mrjones-plip mrjones-plip changed the title Add link checker in CI CI: Add link checker & refactor two files into one Aug 4, 2020
@mrjones-plip mrjones-plip marked this pull request as ready for review August 7, 2020 21:55
@mrjones-plip
Copy link
Contributor Author

mrjones-plip commented Aug 7, 2020

@abbyad - tapping you for a review of this PR, but i see @nomulex did the original architecture (and he's back next week), so feel free to pass back to him if that makes more sense! Never mind! Code owners workflow picked for me!

@nomulex
Copy link
Contributor

nomulex commented Aug 10, 2020

@mrjones-plip do you still need a review here. I have some questions on this section that was deleted.

- name: Install Node depencies	      
        uses: actions/setup-node@v1	
        with:	
          node-version: '12.x'	
      - run: npm install	
      - run: npm install -g postcss-cli	
      - run: npm i -D autoprefixer

Copy link
Contributor

@abbyad abbyad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing the readme only, as Alex will review the rest.

Great to have this change! I've left comments inline to try to clarify the content a bit.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@mrjones-plip
Copy link
Contributor Author

do you still need a review here. I have some questions on this section that was deleted.

@nomulex - yes, I'd love a technical review please! Marc said he'd review my updates to the readme.

Per your question, those lines to install node dependencies weren't deleted, they were moved here. As well, I reformatted them into a single, multi-line run statement.

@nomulex
Copy link
Contributor

nomulex commented Aug 10, 2020

Great @mrjones-plip . I see that hugo build now only applies to the master branch. I recall the initial design requirement was for it to happen for all branches as put here and deploy was for master only. The idea was to detect build failures early enough. If this is no longer needed then everything else looks great. Nice Job!

@mrjones-plip
Copy link
Contributor Author

mrjones-plip commented Aug 10, 2020

I see that hugo build now only applies to the master branch.

@nomulex - thanks for the review! If you look at line 10 you'll see we actually apply this to all branches. And then from line 52 on, we only apply to master using if statements like so:

- name: Master Branch Only - Install Node 12.x
  if: ${{ github.ref == 'refs/heads/master' }}
  uses: actions/setup-node@v1
  with:
      node-version: '12.x'

The effect is that every commit to not master will always have the build checked, including the new muffet link checker. However, only the master branch will actually get pushed live (if build doesn't fail of course ;).

everything else looks great. Nice Job!

Awesome, thanks thanks! If it all looks good and my explanation above makes sense, feel free to mark this review as complete.

@nomulex
Copy link
Contributor

nomulex commented Aug 10, 2020

Apologies, I guess I did not ask my clarifying questions clearly. So when I last worked on this the hugo build here, needed the npm dependencies here to run. This step comes after installing npm dependencies not before like it is here..

@mrjones-plip
Copy link
Contributor Author

Ah - I see, thanks for clarifying!

Actually the npm dependencies are not needed to run hugo and are not needed to run hugo server&. They're only needed for when we run env HUGO_ENV="production" hugo --minify. This is because that's the only call that uses the postcss-cli and autoprefixer npm libraries.

If I'm still missing something (sorry if so!), maybe it'd be easier to hop on a google meet call to sort it out real time? That said, I see it's late where you are, so this can wait 'til tomorrow if need be! No big rush on this.

@mrjones-plip
Copy link
Contributor Author

@abbyad - I think all the changes you requested to the readme.md are tended to - lemme know if not!

@abbyad
Copy link
Contributor

abbyad commented Aug 10, 2020

@mrjones-plip, I think you make a good point here: #266 (comment)

The instructions listed in the README are for devs... but it is not clear why and how the current headers relate. @MaxDiz what do you think about restructuring it as part of another issue?

@abbyad abbyad self-requested a review August 10, 2020 22:07
Copy link
Contributor

@abbyad abbyad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The readme looks good from my side - nice work!

@mrjones-plip
Copy link
Contributor Author

The instructions listed in the README are for devs... but it is not clear why and how the current headers relate. @MaxDiz what do you think about restructuring it as part of another issue?

I actually wonder, since we have developer instructions in our docs, if we should add more to the current "Getting Started" section there? We don't explicitly call out the difference between making edits on GH directly (or in your own fork) vs setting up hugo locally.

So yeah, my vote would be to spin up a new ticket to spice up the the Basics section to include this. Or, yeah, the readme ;)

@mrjones-plip mrjones-plip requested a review from abbyad August 10, 2020 22:54
Copy link
Contributor

@nomulex nomulex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mrjones-plip mrjones-plip merged commit 6f09e16 into master Aug 11, 2020
@mrjones-plip
Copy link
Contributor Author

@garethbowen , @abbyad & @nomulex - Much thanks for all your help with this PR! The first full build against master seemed to go OK!

@MaxDiz
Copy link
Contributor

MaxDiz commented Aug 11, 2020

created a new issue: #275 based on comment above. Feel free to add to it

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

Successfully merging this pull request may close these issues.

CI: Add link checker to avoid 404s going live, merge CI docs into one to simplify
4 participants