greenelab / lab-website-template

An easy-to-use, flexible website template for labs.
https://greenelab.github.io/lab-website-template/
BSD 3-Clause "New" or "Revised" License
364 stars 315 forks source link

V1.2.0 #244

Closed vincerubinetti closed 8 months ago

vincerubinetti commented 8 months ago

New template version checklist:

github-actions[bot] commented 8 months ago

PR Preview Action v1.4.7 :---: Preview removed because the pull request was closed. 2024-03-08 21:30 UTC

vincerubinetti commented 8 months ago

This is ready for review. Regarding the failing pull request preview workflow, I'm fairly certain that this is due to GitHub wanting to run the version of the workflow that is on main, not this branch. If you look at the failing logs, it says Run ruby/setup-ruby@v1 with: ruby-version: 3.0, which is what is currently on main, instead of 3.1 which I've set in this PR. Weirdly, if I clear the gh-actions Ruby cache and re-run the failed job, it seems to pass, but then the next run fails.

Regardless, I've tested this on my own repo, as if the v1.2.0 changes were already on main, and it seems to work fine: https://github.com/vincerubinetti/lwt-test/pull/1 The other workflows also seem to work fine with the version upgrades.

So, I think it is safe to ignore the failure, as it should work properly once merged into main.

falquaddoomi commented 8 months ago

This is ready for review. Regarding the failing pull request preview workflow, I'm fairly certain that this is due to GitHub wanting to run the version of the workflow that is on main, not this branch. If you look at the failing logs, it says Run ruby/setup-ruby@v1 with: ruby-version: 3.0, which is what is currently on main, instead of 3.1 which I've set in this PR. Weirdly, if I clear the gh-actions Ruby cache and re-run the failed job, it seems to pass, but then the next run fails.

Regardless, I've tested this on my own repo, as if the v1.2.0 changes were already on main, and it seems to work fine: vincerubinetti/lwt-test#1 The other workflows also seem to work fine with the version upgrades.

So, I think it is safe to ignore the failure, as it should work properly once merged into main.

About it running the version in main and not your PR branch: what you said is correct; because you're using pull_request_target as the event, by design it runs in the base branch (i.e., not the PR branch). From the docs on pull_request_target:

This event runs in the context of the base of the pull request, rather than in the context of the merge commit, as the pull_request event does. This prevents execution of unsafe code from the head of the pull request that could alter your repository or steal any secrets you use in your workflow. This event allows your workflow to do things like label or comment on pull requests from forks. Avoid using this event if you need to build or run code from the pull request.

Anyway, I'll ignore the failure like you said. I'll review this ASAP, too.

vincerubinetti commented 8 months ago

Yes, I temporarily tried adding the regular pull_request target here: https://github.com/greenelab/lab-website-template/pull/244/commits/b5cacf253c41c092f1ef5016ec1ca4d821ef81ff

~But that also seemed to not work for some reason, unless I was looking at the wrong workflow run.~

Based on testing updating the greenelab.com website, adding a pull_request trigger to on-pull-request.yaml (in the same manner as pull_request_target) works. Any user wanting to have a live deploy preview for a PR when updating their site to v1.2.0 can temporarily add this, then remove it again before merge.

Regardless, we need pull_request_target because in previous testing it proved to be more reliable when forking the template, and we cannot have both triggers because it will create confusing duplicate checks on PRs (and one of them usually fails while the other one passes, depending on whether the change is being made from a branch or a fork).

vincerubinetti commented 8 months ago

Great work! I left a few comments, many of them optional/minor. Let me know if you have any questions.

Along the way I wondered if a CSS or SCSS linter could assist over time (if they're not already present). In addition, perhaps a Liquid linter could help in a similar way if it exists. Overall I have gaps in my understanding for those, so it could be that this is impractical or not relevant. I bring it up as a thought to strengthen the work and am not concerned (I think you did a good job on these things from what I can tell).

As far as I know, there's no actively maintained linter for liquid. I did investigate using a liquid formatter at least, which should be somewhere in the issue/pr history, but I couldn't get it to work reliably with the mix of markdown, html, and liquid.

Regarding CSS, I do have IDE extensions that will tell me for example if I've written an invalid CSS property or value for it, but of course that doesn't cover other people who might maintain it in the future. Perhaps I could add a .vscode folder with recommended extensions and settings at some point. It would also be possible to set up PR tests for linting and such, however the template really hasn't opened that can of worms yet for anything else. It's assumed most users won't change the template internals (css/js/etc), and if they do, it's on them. Bluntly, Jekyll/Liquid/Ruby imo are just not really the right or modern tools for the job. Simply using JSX and TS and components with a framework like Astro would alleviate so many problems, e.g. automatic documentation for what components take what props.

vincerubinetti commented 8 months ago

Also I forgot to mention, I have a gitbook "pr" ready to merge for these changes. I believe you should be able to see this preview link:

https://greene-lab.gitbook.io/lab-website-template-docs/~/changes/i7RDBjuWCIkfNmYxvoTx/introduction/overview