Skip to content

Assign ZIP 2001 to draft-nuttycom-lockbox-streams#887

Merged
str4d merged 3 commits into
mainfrom
zip-2001
Aug 1, 2024
Merged

Assign ZIP 2001 to draft-nuttycom-lockbox-streams#887
str4d merged 3 commits into
mainfrom
zip-2001

Conversation

@str4d

@str4d str4d commented Jul 30, 2024

Copy link
Copy Markdown
Collaborator

Also attempts to add a redirect from the old URL to the new one. We won't know if this worked until this PR is merged.

This was missed in the refactor PR.
Comment thread zips/draft-nuttycom-funding-allocation.rst Outdated
Comment thread zips/draft-zf-community-dev-fund-2-proposal.rst Outdated
Comment on lines 1 to 3

@daira daira Jul 31, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm almost sure that this won't work. GitHub pages will only serve a static site. The sites that use Jinja have a GitHub Action that renders the template to HTML; here you're trying to include the template in the HTML.

I think this will work:

Suggested change
---
redirect_to: "https://zips.z.cash/zip-2001"
---
<!DOCTYPE html>
<html>
<head>
<title>ZIP 2001: Lockbox Funding Streams (redirect)</title>
<meta http-equiv="refresh" content="0; https://zips.z.cash/zip-2001">
</head>
</html>

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It was unclear to me how GitHub Pages Jinja was configured. The existence of _config.yml in the repo implied that you were already using it, and AFAICT the way you configure Jinja for a given page is to prepend YML to the HTML.

That being said, indeed this particular gem is intended to generate the equivalent of the HTML you've given, so if the Jinja stuff doesn't work, then we should a) use your suggestion, and b) delete _config.yml to stop it being misleading.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think _config.yml is probably a leftover, but I could be wrong.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We will find out one way or another once this PR merges.

Comment thread rendered/_config.yml Outdated
Comment on lines 4 to 5

@daira daira Jul 31, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is unnecessary (and will not work). It would only have an effect when rendering the HTML; see my other comment.

Suggested change
gems:
- jekyll-redirect-from

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

If this doesn't work, then the entire _config.yml is doing nothing, because that config file is for Jinja.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

https://github.com/orgs/community/discussions/53549 seems to indicate that Jinja is always running because GitHub Pages is powered by it, which would imply that gems do work.

@daira daira left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Changes needed. In particular I'm almost sure the redirect won't work, and I have suggested an alternative.

@daira daira left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Unblocking merge.

@str4d

str4d commented Aug 1, 2024

Copy link
Copy Markdown
Collaborator Author

Force-pushed to address @daira's comments.

@str4d str4d merged commit 6f1a2ea into main Aug 1, 2024
@str4d str4d deleted the zip-2001 branch August 1, 2024 15:01
@daira

daira commented Aug 1, 2024

Copy link
Copy Markdown
Collaborator

Post-hoc ACK

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.

2 participants