search

bf2fc6cc711aee1a0c2a / architecture

Repository containing the architecture documents.
https://architecture.bf2.dev/
Apache License 2.0
5 stars 20 forks source link

Update to Jekyll 4, fix some other things #56

Closed grdryn closed 2 years ago

grdryn commented 2 years ago

GitHub pages has been stuck for years on an old version of Jekyll. Updating their convenience tooling to Jekyll 4 would break a lot of existing sites, so it probably will never happen. See here for more info: https://github.com/github/pages-gem/issues/651

It turns out that we're already using the Jekyll GitHub Action (I'm not sure what the reason we've been doing that already is), so there's little reason not to upgrade.

This closes #47 by adding instructions for building locally using a container image. This also closes #49 by adding liquid pre-processing explicitly in the file where it was missing.

See individual commit messages for more fine-grained details of other changes. Additionally, reviewing will be easier with the ?w=1 query param: https://github.com/bf2fc6cc711aee1a0c2a/architecture/pull/56/files?w=1

These changes can be seen here, which is build from this branch of my fork, which just contains one additional commit on top of the source branch of this PR, to get everything to work nicely for the different domain, etc.

grdryn commented 2 years ago

@tombentley @wtrocki @ziccardi Please take a look, thanks! :slightly_smiling_face:

grdryn commented 2 years ago

Thanks for the review @tombentley!

BTW one thing that occurred to me when creating this PR is that normies like myself don't have the ability to add reviewers to our PRs here. Just calling it out in case it's not intentional :slightly_smiling_face:

tombentley commented 2 years ago

I don't have the ability to change it, and I think it might be the model we want for most of the PRs which are about adding content (we can be sure the right people review each ADR/AP/PADR).

wtrocki commented 2 years ago

Was on PTO. This works locally. @grdryn Anything else you would want me to change.

wtrocki commented 2 years ago

@grdryn we can add extra maintainers in org-management (write permission is needed to be reviewer)

grdryn commented 2 years ago

Thanks for reviewing & verifying @wtrocki!

@grdryn Anything else you would want me to change.

I don't think so, thanks. I think the build + deploy should work as-is. :pray:

@grdryn we can add extra maintainers in org-management (write permission is needed to be reviewer)

I don't really have any strong opinions on who should review (or be able to request review). As @tombentley mentioned above, it might be intentional for now that we can't set reviewers. I guess in this new process, us proposers can still @-mention specific people to try to get their attention, and then it's up to the architects to formally add reviewers?

In either case, I think this can be merged now, if y'all are happy :slightly_smiling_face:

wtrocki commented 2 years ago

Merged so we can test this on CI/CD level

wtrocki commented 2 years ago

Updated everything smoothly. website looks ok. Amazing work @grdryn

tombentley commented 2 years ago

Many thanks for this one @grdryn!