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

Fail and cancel deploy on Franklin Warning #1364

Merged
merged 7 commits into from
Aug 26, 2021
Merged

Fail and cancel deploy on Franklin Warning #1364

merged 7 commits into from
Aug 26, 2021

Conversation

rikhuijzer
Copy link
Contributor

@rikhuijzer rikhuijzer commented Aug 26, 2021

As discussed in #1357.

Fixes #1357

This is the hacky and quick fix. It greps the logs, for "Franklin Warning". So, this assumes that Franklin prints the text "Franklin Warning" when something bad happens. In my experience, it works quite well because any parse or runtime error will give this warning. However, note that this doesn't detect missing images and/or probably other problems. For my own blog, this grep hack has worked for a year now and works quite well. CI has failed quite a lot of times without me expecting that it would fail, which is a good thing.

Note that the output isn't printed immediately to stdout and only to build.log. In my experience, that's fine. The only difference is that you have to wait a few more minutes before the stdout becomes visible.

Also, I've added some whitespace between the steps for readability.

@rikhuijzer rikhuijzer marked this pull request as draft August 26, 2021 14:47
@rikhuijzer
Copy link
Contributor Author

rikhuijzer commented Aug 26, 2021

Okay, it doesn't fail now for some reason, but this approach should work. The logs of the breaking post contain "Franklin Warning" (https://github.com/JuliaLang/www.julialang.org/runs/3383439513):

┌ Franklin Warning: in <index.md>
│ Encountered an issue processing 'index.md' in w.julialang.org/blog.
│ Verify, then re-start the Franklin server.
│ The error is displayed below:
│ Franklin.OCBlockError("I found the opening token 'MATH_A' but not the corresponding closing token.", "Context:\n\t... this year, raising ~\$4,000! If you missed ... (near line 32)\n\t                        ^---\n")

@rikhuijzer rikhuijzer marked this pull request as ready for review August 26, 2021 14:54
Copy link
Contributor

@giordano giordano left a comment

Choose a reason for hiding this comment

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

Let's add some colour

.github/workflows/deploy.yml Outdated Show resolved Hide resolved
.github/workflows/deploy.yml Outdated Show resolved Hide resolved
Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
@DilumAluthge
Copy link
Member

Just to test this out, can you intentionally introduce the kind of syntax error that #1356 had, and we can demonstrate that CI then fails.

@DilumAluthge
Copy link
Member

DilumAluthge commented Aug 26, 2021

Ah, I see you kind of tried that out above.

Maybe try a different kind of syntax error?

It would definitely be good to show that we can make CI fail with this.

@DilumAluthge
Copy link
Member

@tlienart Any suggestions for a syntax error that we can push to this PR to demonstrate that CI fails?

@tlienart
Copy link
Sponsor Contributor

tlienart commented Aug 26, 2021

Note in passing that in the time we've had the site on Franklin the main issue I'm aware of is the $ sign not being escaped. So e.g. grant body XXX gave us $5000 instead of grant body XXX gave us \$5000 so if the deploy script here can catch that then it's already a big step in my opinion (and hopefully will last long enough for me to throw the error on the Franklin side)

@DilumAluthge
Copy link
Member

I just pushed a commit that adds the text "one dollar plus one dollar equals $2" to one of the markdown files.

@DilumAluthge
Copy link
Member

Success!

@DilumAluthge
Copy link
Member

@rikhuijzer @tlienart Once CI is green, is this good to merge?

@tlienart
Copy link
Sponsor Contributor

Good stuff! merging, thanks @rikhuijzer and to all for the reviews :)

@tlienart tlienart merged commit 3646314 into JuliaLang:main Aug 26, 2021
@DilumAluthge
Copy link
Member

Thank you @rikhuijzer!

@rikhuijzer rikhuijzer deleted the grep-warning branch March 28, 2022 20:43
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: site generation failure should block PR merges
4 participants