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

Chore/wireframe preview ci #2026

Merged
merged 12 commits into from
Jul 20, 2021
Merged

Chore/wireframe preview ci #2026

merged 12 commits into from
Jul 20, 2021

Conversation

mnyrop
Copy link
Member

@mnyrop mnyrop commented Jul 19, 2021

testing a whole slew of changes to make CI and preview builds more flexible.
very experimental – don't merge!

@mnyrop mnyrop self-assigned this Jul 19, 2021
@mnyrop
Copy link
Member Author

mnyrop commented Jul 19, 2021

Hi @glenrobson + @hadro! This is a massive and somewhat invasive PR – apologies!

I'll explain what's happening below and, if/when it's approved, I'll abstract the Rake tests out of this repo and move them into the theme repo (or a "IIIF/site-tests" repo – whichever you prefer). But before that, I think this is a good time to pause and get your feedback.

Major changes:

  • Content has been moved out of the /api sub-directory and the default baseurl /api is used instead.
    This provides greater flexibility, ensures theme/assets sync to the proper root, and ensures nothing deploys to the root path that shouldn't. (One side-effect to note: previews will not have the extra /api at the end.)
  • Special URL paths (e.g., {{ site.root_url }}, {{ site.api_url }}) need to be passed through the | absolute_url filter at some point in order to receive the ci, preview or any other baseurl.
    Many includes will do this as part of their own process (e.g., the navbar header), but regular links (e.g., in links.md) will need to add this filter explicitly. Passing a link through | absolute_url more than once will result in a doubling of the baseurl and will flag as an error in the test suite, so this should be relatively straight-forward to catch.
  • The tests have totally been rewritten, including a build:preview and more granular, name-spaced testing tasks (i.e., test:html, test:links:internal, test:links:iiif, test:links:external, test:spec)
    I can go into more detail for these when we meet if helpful.
  • The preview.yml action currently runs the tests on the actual preview build before deployment, rather than on an arbitrary CI /test build.
    That said, the bundle exec rake ci task still exists as intended and allows nested directories for testing too.

Please take your time playing around with the branch + asking questions – I know it's a lot. I'll switch over to UI features in the meantime.

@glenrobson
Copy link
Member

Thanks @mnyrop, this is sounding great. Ill do a deep dive tonight and will get back to you with questions.

@glenrobson
Copy link
Member

I've been through the GitHub actions file and Rakefile and its looking great. A lot cleaner than the previous way of doing it. I can also see how to create a build:live rake task and github action file when we get to that point.

One question I came up with, was have we lost the JSON validation for the API site? This would only run for the API site so maybe something that wouldn't go into the theme. If we want to re-add it should we add the following line back (

api/Rakefile

Line 21 in 6d29fdd

sh 'scripts/check_json.py -v'
) or is there a better way? Note this might answer the scripts directory question.

Also the test:spec is only relevant to the API site. Is there a way to have this task only in the API site and not in the theme? Not a big problem if not as we just won't run it for the other sites.

@glenrobson
Copy link
Member

Sorry one more thing, in the preview.yml it says:

uses: actions/checkout@v2 # store branch name in ${BRANCH_NAME}

Does this action store the branch name in ${BRANCH_NAME}? If so we could get rid of the nelonoel/branch-name@v1.0.1 action.

@mnyrop
Copy link
Member Author

mnyrop commented Jul 20, 2021

Hi Glen, I'll reply 1 by 1:

One question I came up with, was have we lost the JSON validation for the API site? This would only run for the API site so maybe something that wouldn't go into the theme. If we want to re-add it should we add the following line back (

api/Rakefile

Line 21 in 6d29fdd

sh 'scripts/check_json.py -v'

) or is there a better way? Note this might answer the scripts directory question.

Yes, I can re-add this as a separate task in the API repo Rakefile only. It would be great to rewrite it in Ruby & fold it into the same RSpec workflow, but that can be a stretch goal at a later point.

Also the test:spec is only relevant to the API site. Is there a way to have this task only in the API site and not in the theme? Not a big problem if not as we just won't run it for the other sites.

Yes, that's totally possible. I'll keep it in the API Rakefile and can namespace it as test:api:spec to make its scope clearer.

Sorry one more thing, in the preview.yml it says:

uses: actions/checkout@v2 # store branch name in ${BRANCH_NAME}

Does this action store the branch name in ${BRANCH_NAME}? If so we could get rid of the nelonoel/branch-name@v1.0.1 action.

No, sorry! That comment should be on the line below–I must've messed it up when I was reworking the white space. I'll fix it a quick commit on this branch. That said, in the Rakefile I get the branch name by running sh "git rev-parse --abbrev-ref HEAD". I can replace that action with that command to make things more minimal if you'd like...? But we still need one or the other.


So to-dos for me on this branch include:

  • Fix comment in preview.yml
  • (Re)add /scripts tasks
  • Namespace /scripts tasks and spec task under api:test
  • Abstract other tasks out into the theme (or testing) gem

(EDIT: the above are completed in #2031)

Are there other to-dos you can think of?
And:

1. Can I remove /config ? Nevermind! I just rebased from wireframe and can see it was removed.
2. Should I replace nelonoel/branch-name@v1.0.1 with git rev-parse --abbrev-ref HEAD ?
--> yes
3. Would you prefer the shared tests/tasks live in the theme or a more modular IIIF/site-tests repo?
--> theme repo

@glenrobson
Copy link
Member

I agree it would be nice to move the JSON check to Ruby but it could be done later. Regarding your other questions:

  1. Should I replace nelonoel/branch-name@v1.0.1 with git rev-parse --abbrev-ref HEAD ?

Were you thinking of something like:

export BRANCH_NAME=`git rev-parse --abbrev-ref HEAD`

so that we can use the variable further down in the YAML file? If so yes that would be great to remove one of the actions and simplify and possibly quicken the build process.

  1. Would you prefer the shared tests/tasks live in the theme or a more modular IIIF/site-tests repo?

I lean to having the shared tasks in the theme but open to suggestions. I think less repos is generally desirable and I can't see we would use them outside of the theme. I'm hoping we can use the theme for all of the Jekyll sites we run.

@mnyrop
Copy link
Member Author

mnyrop commented Jul 20, 2021

@glenrobson re: the BRANCH_NAME. yes, I think that's exactly the way to do it. I'll add that now.

Should I correct the merge conflict below and merge now so that wireframe and this branch don't fall out of sync?

I can work on moving shared tasks/tests out to the theme after that.

@glenrobson
Copy link
Member

I've fixed the conflict. Did you want to merge then move the rake tasks to the theme or move the rake tasks then merge? I'm fine with either option and if its the first feel free to merge this into Wireframe.

@mnyrop mnyrop merged commit 1c0f8e5 into wireframe Jul 20, 2021
@mnyrop mnyrop deleted the chore/wireframe-preview-ci branch July 20, 2021 22:11
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.

None yet

2 participants