Add build-docs job to main github actions file by exequiel09 · Pull Request #8504 · Leaflet/Leaflet

exequiel09 · 2022-10-01T11:58:57Z

jonkoops

This is a welcome change. Great work! I just have one small comment.

Co-authored-by: Jon Koops <jonkoops@gmail.com>

jonkoops

Sorry for the additional review comments, but looks like we can optimize things a bit.

mourner · 2022-10-01T16:05:46Z

@exequiel09 it's now failing with a surprising error about not being able to build some internal example for some reason...

mourner · 2022-10-01T17:16:21Z

@exequiel09 looks like jekyll is available, but it still shows that error...

exequiel09 · 2022-10-01T17:25:55Z

Maybe related to this. still reading docs: https://jekyllrb.com/docs/troubleshooting/#configuration-problems

exequiel09 · 2022-10-01T17:33:44Z

It's really hard debugging this pipeline issue 😅

exequiel09 · 2022-10-01T17:35:12Z

Finally! It worked https://github.com/Leaflet/Leaflet/actions/runs/3165492356/jobs/5154569243 🎊

Falke-Design

The building works but it doesn't break when the error happens:

https://github.com/Falke-Design/Leaflet/actions/runs/3165535999/jobs/5154637242

jonkoops · 2022-10-01T18:05:55Z

@@ -1,4 +1,4 @@
-exclude: [build, debug, node_modules, spec, src, CNAME, reference-tpl.html, CHANGELOG.md, README.md, LICENSE]
+exclude: [build, debug, node_modules, spec, src, CNAME, reference-tpl.html, CHANGELOG.md, README.md, LICENSE, vendor/bundle, vendor/cache, vendor/gems, vendor/ruby]


Just thinking out loud here, perhaps it would be better to use an include rather than to list all the things that should be excluded?

jonkoops

Code changes look good to me, but I am interested why it doesn't fail the job as expected when an error is introduced as @Falke-Design mentioned.

jonkoops · 2022-10-01T18:08:51Z

It's really hard debugging this pipeline issue 😅

Yeah, debugging stuff on Github Actions can be a bit of a hassle. I usually test on my own fork as well, less waiting on upstream capacity.

exequiel09 · 2022-10-01T18:15:33Z

oh. I didn't know that I can run GH actions on a fork 😲

exequiel09 · 2022-10-01T18:25:16Z

I usually test on my own fork as well, less waiting on upstream capacity.

Not sure on how to run on my own fork. Quick search landed me on the event pull_request_target: https://dev.to/github/running-github-actions-on-forks-4e75

exequiel09 · 2022-10-01T18:28:11Z

Okay so running locally, somehow error is swallowed. Running echo $? after the failed command yield exit status of 0 which it shouldn't be.

exequiel09 · 2022-10-01T18:30:55Z

bundle exec executes a command in the context of the bundle (from bundle exec --help ).
If you pass a executable name that is not found, e.g. bundle exec thisisnotwhatyouarelookingfor, it will exit with a status code != 0.

https://stackoverflow.com/a/64785111

I think I need to do some digging further to bubble up the error

Falke-Design · 2022-10-01T18:32:48Z

Can we store the output on a file and check if it contains Error?

Falke-Design · 2022-10-01T18:35:35Z

Found a repo: https://github.com/epstechtheatre/Jekyll-Tester

exequiel09 · 2022-10-01T18:45:17Z

Found a repo: https://github.com/epstechtheatre/Jekyll-Tester

Pushed commit to use the suggestion action 6edc829

exequiel09 · 2022-10-01T18:57:11Z

Seems like the suggested action for jekyll doesn't work.

This line was never emitted https://github.com/epstechtheatre/Jekyll-Tester/blob/5331113e1372cd3a625eaa7398e334fe21397507/src/index.ts#L91

in this fork run: https://github.com/exequiel09/Leaflet/actions/runs/3165731159/jobs/5154974801#step:5:141

exequiel09 · 2022-10-01T19:09:24Z

Okay so it's not related to bundler but with jekyll: jekyll/jekyll#8367

exequiel09 · 2022-10-01T19:10:57Z

Okay I know the fix. Pushing another one. removing the suggested action as well

… proper exit status

exequiel09 · 2022-10-01T19:15:03Z

Added --strict_front_matter.

– https://stackoverflow.com/a/68600279

exequiel09 · 2022-10-01T19:17:00Z

Adding --strict_front_matter will make exit status be 1

Falke-Design

I have no idea how the workflows are working but the output looks good now 😄

jonkoops

Workflow is looking good to me, got a couple small questions but after that I think we can merge.

jonkoops · 2022-10-01T20:00:09Z

@@ -1,4 +1,4 @@
-exclude: [build, debug, node_modules, spec, src, CNAME, reference-tpl.html, CHANGELOG.md, README.md, LICENSE]
+exclude: [build, debug, node_modules, spec, src, CNAME, reference-tpl.html, CHANGELOG.md, README.md, LICENSE, vendor/bundle, vendor/cache, vendor/gems, vendor/ruby]


Do we really need anything from the vendor directory? Perhaps we can exclude all of it?

Suggested change

exclude: [build, debug, node_modules, spec, src, CNAME, reference-tpl.html, CHANGELOG.md, README.md, LICENSE, vendor/bundle, vendor/cache, vendor/gems, vendor/ruby]

exclude: [build, debug, node_modules, spec, src, CNAME, reference-tpl.html, CHANGELOG.md, README.md, LICENSE, vendor]

I'm not sure on the proper solution but the jekyll website suggested that fix on CI issue #8504 (comment)

There maybe folders that jekyll team don't want to ignore hence they added a blacklist of it

Mentioned in jekyll website:

The proper solution is to incorporate the default setting for exclude: rather than override it completely:
For versions up to v3.4.3, the exclude: setting must look like following:

exclude: - Gemfile - Gemfile.lock - node_modules - vendor/bundle/ - vendor/cache/ - vendor/gems/ - vendor/ruby/ - any_additional_item # any user-specific listing goes at the end

exequiel09 · 2022-10-02T01:06:15Z

I have no idea how the workflows are working but the output looks good now 😄

@Falke-Design we need the build to command to return non-zero exit status to make GitHub actions check fail. In that case jekyll build returns 1 when there are errors and 0 if there's none.

mourner

Nice work!

jonkoops

Looking good to me!

jonkoops · 2022-10-02T15:35:59Z

Thank you so much for this contribution @exequiel09. This change helps us out a lot!

exequiel09 added 2 commits October 1, 2022 19:57

Add build-docs job to main github actions file

a0111e1

Ensure jekyll is installed

62a8157

jonkoops requested changes Oct 1, 2022

View reviewed changes

Comment thread .github/workflows/main.yml Outdated

jonkoops assigned exequiel09 Oct 1, 2022

jonkoops added the workflow Anything related to Leaflet development process and tools label Oct 1, 2022

Remove cache-version from ruby/setup-ruby

200fbcb

Co-authored-by: Jon Koops <jonkoops@gmail.com>

exequiel09 requested a review from jonkoops October 1, 2022 13:57

jonkoops requested changes Oct 1, 2022

View reviewed changes

Comment thread .github/workflows/main.yml Outdated

Comment thread .github/workflows/main.yml

Comment thread .github/workflows/main.yml Outdated

Comment thread .github/workflows/main.yml Outdated

Comment thread .github/workflows/main.yml Outdated

Comment thread .github/workflows/main.yml

exequiel09 added 2 commits October 1, 2022 22:30

Remove node related setup and add jekyll to Gemfile

9ef05f3

Remove jekyll gem install command

4bb4600

exequiel09 requested a review from jonkoops October 1, 2022 14:33

mourner reviewed Oct 1, 2022

View reviewed changes

Comment thread docs/Gemfile Outdated

Remove jekyll

160d510

Exclude vendor dirs

d48af9f

exequiel09 requested review from jonkoops and mourner and removed request for jonkoops and mourner October 1, 2022 17:32

Falke-Design requested changes Oct 1, 2022

View reviewed changes

jonkoops reviewed Oct 1, 2022

View reviewed changes

jonkoops approved these changes Oct 1, 2022

View reviewed changes

Use epstechtheatre/Jekyll-Tester action

6edc829

exequiel09 requested a review from Falke-Design October 1, 2022 18:47

Add missing node installation required by epstechtheatre/Jekyll-Tester

6e495bf

exequiel09 commented Oct 1, 2022

View reviewed changes

Comment thread .github/workflows/main.yml Outdated

Revert suggested action and --strict_front_matter to make jekyll emit…

5c73cd5

… proper exit status

Falke-Design requested a review from jonkoops October 1, 2022 19:33

Falke-Design approved these changes Oct 1, 2022

View reviewed changes

jonkoops requested changes Oct 1, 2022

View reviewed changes

exequiel09 added 2 commits October 2, 2022 09:02

Undo changes to Gemfile

694d67a

Ignore generated .jekyll-cache folder inside docs

55ee646

mourner approved these changes Oct 2, 2022

View reviewed changes

jonkoops approved these changes Oct 2, 2022

View reviewed changes

jonkoops merged commit f3d8a05 into Leaflet:main Oct 2, 2022

exequiel09 deleted the jekyll-github-actions-check-#8502 branch October 2, 2022 15:43

		@@ -1,4 +1,4 @@
		exclude: [build, debug, node_modules, spec, src, CNAME, reference-tpl.html, CHANGELOG.md, README.md, LICENSE]
		exclude: [build, debug, node_modules, spec, src, CNAME, reference-tpl.html, CHANGELOG.md, README.md, LICENSE, vendor/bundle, vendor/cache, vendor/gems, vendor/ruby]

Uh oh!

Conversation

exequiel09 commented Oct 1, 2022

Uh oh!

jonkoops left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonkoops left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mourner commented Oct 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mourner commented Oct 1, 2022

Uh oh!

exequiel09 commented Oct 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

exequiel09 commented Oct 1, 2022

Uh oh!

exequiel09 commented Oct 1, 2022

Uh oh!

Falke-Design left a comment

Choose a reason for hiding this comment

Uh oh!

jonkoops Oct 1, 2022

Choose a reason for hiding this comment

Uh oh!

jonkoops left a comment

Choose a reason for hiding this comment

Uh oh!

jonkoops commented Oct 1, 2022

Uh oh!

exequiel09 commented Oct 1, 2022

Uh oh!

exequiel09 commented Oct 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

exequiel09 commented Oct 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

exequiel09 commented Oct 1, 2022

Uh oh!

Falke-Design commented Oct 1, 2022

Uh oh!

Falke-Design commented Oct 1, 2022

Uh oh!

exequiel09 commented Oct 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

exequiel09 commented Oct 1, 2022

Uh oh!

exequiel09 commented Oct 1, 2022

Uh oh!

exequiel09 commented Oct 1, 2022

Uh oh!

exequiel09 commented Oct 1, 2022

Uh oh!

exequiel09 commented Oct 1, 2022

Uh oh!

Falke-Design left a comment

Choose a reason for hiding this comment

Uh oh!

jonkoops left a comment

Choose a reason for hiding this comment

Uh oh!

jonkoops Oct 1, 2022

Choose a reason for hiding this comment

Uh oh!

exequiel09 Oct 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mourner commented Oct 1, 2022 •

edited

Loading

exequiel09 commented Oct 1, 2022 •

edited

Loading

exequiel09 commented Oct 1, 2022 •

edited

Loading

exequiel09 commented Oct 1, 2022 •

edited

Loading

exequiel09 commented Oct 1, 2022 •

edited

Loading

exequiel09 Oct 2, 2022 •

edited

Loading

exequiel09 Oct 2, 2022 •

edited

Loading