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

Introduce previews on Actions #1193

Merged
merged 9 commits into from Nov 5, 2020
Merged

Introduce previews on Actions #1193

merged 9 commits into from Nov 5, 2020

Conversation

mehmetoguzderin
Copy link
Member

No description provided.

kainino0x
kainino0x previously approved these changes Oct 30, 2020
Copy link
Contributor

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

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

Using the bikeshed api here is really clever and I totally didn't think whether that could help us here! (I actually didn't know it had a live preview mode like this.) I tested this out on Oguz's test repo and seemed like it will work great. Love the simplicity!

Something we discussed on chat: I do think it will be convenient to just post a comment once, on the first PR upload, that points to the branch head rather than pointing to the hash of a specific revision. I don't think we really need the history in the comments, it will be a bit noisy, and people can easily edit the preview url to point at an old revision if they want to look at it.

@mehmetoguzderin
Copy link
Member Author

mehmetoguzderin commented Oct 30, 2020

@kainino0x Doing so would require creating another specialized-trigger action to run only when someone opens a PR (hence, Preview PR would depend on it to execute). Would that be an acceptable change?

@kainino0x
Copy link
Contributor

Is that more complicated than what you have so far in this PR? I prefer to keep things as simple as possible.

@mehmetoguzderin
Copy link
Member Author

@kainino0x Difference between this and that: https://github.com/mehmetoguzderin/gpuweb/pull/12/files

@kainino0x
Copy link
Contributor

kainino0x commented Oct 30, 2020

Shouldn't you only need to trigger on pull_request opened or am I misunderstanding how this config works? (I have never looked at any of this before, I have no idea)

@kainino0x
Copy link
Contributor

oh, I see that's for the publish step. Sorry, I don't know enough about github actions to know what to do here. Maybe @kvark @pjoe will have more informed opinions about what to do here.

@mehmetoguzderin
Copy link
Member Author

mehmetoguzderin commented Oct 30, 2020

@kainino0x I think both ways are cool (though restricting to open only could be nicer if GitHub Actions did pass the type to event context- but that's a bit too much to expect given that it doesn't even give PR ID, which I extract through search).

The current version has the mental model of a dialogue- let's say a discussion is going on and you did a push, GitHub bot comes and brings up in a "Hey, the preview has changed- you might want to check out" kind of way.

The other version has the mental model of the bot's comment being an appendix to the OP.

@kainino0x
Copy link
Contributor

Agree with the mental models, those are exactly what I was thinking. I don't know which is more practical but we can always try one out and see what we think!

@dneto0
Copy link
Contributor

dneto0 commented Oct 30, 2020

This looks really cool. I just started looking at GitHub actions and I'm learning a lot by looking at your examples.

@mehmetoguzderin
Copy link
Member Author

@dneto0 thanks! I am glad that they help.

kvark
kvark previously approved these changes Nov 2, 2020
@kainino0x
Copy link
Contributor

kainino0x commented Nov 2, 2020

Hey @plinss @tabatkins, we're considering this automation which will generate links like:

https://api.csswg.org/bikeshed/?url=https://raw.githubusercontent.com/mehmetoguzderin/gpuweb/08c6b1618b66f60c47566a619253e20914710e8c/spec/index.bs
and post them as comments to pull requests. These links are extremely convenient, but appear to trigger a full build of the spec on the api.csswg.org server each time (with no caching?*). Do you think it's okay, server-load-wise, that we would generate a lot of these links, each reviewer would click them, and also that they might get accessed by web crawlers? (I'm also not sure who pays for that API to run.)

* currently takes about 40 seconds (hm, I thought this was a bit faster a few days ago). Although, when timing it just now I got an 500 error on 2 of my 5 attempts, after what looked like a 60s timeout.

Thanks for your input!

@plinss
Copy link

plinss commented Nov 3, 2020

Hi @kainino0x, please do not do that.

The Bikeshed API server is meant as a service for continuous integration processes and specification authors who want to check their work when a locally installed copy of bikeshed isn't available.

Putting links like that out into the wild will put an unacceptable burden on the server and would cause me to require a POST on the API endpoint (or possibly restrict the service to logged-in users).

The Bikeshed API server does not cache by design. It's not meant to serve content to end users.

You'd be better served by running a CI process to build the PRs and serve them elsewhere.

It'd be fine to have your automation call the API server once per PR (and again for further commits on that PR) and store the result elsewhere, then link to that for reviewers.

(BTW: thanks for asking before doing this)

@kainino0x
Copy link
Contributor

Thank you for the info! We will find another solution :)

@mehmetoguzderin
Copy link
Member Author

I am looking into updating this PR to use Firebase integration instead.

@kainino0x kainino0x dismissed stale reviews from kvark and themself November 3, 2020 02:06

Going to try a different approach

@mehmetoguzderin
Copy link
Member Author

The current version (as of last push) uses Firebase Hosting to serve preview files and does not communicate with Bikeshed API to render the specs.

Copy link
Contributor

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Great work! A few notes

.github/workflows/preview.yml Show resolved Hide resolved
.github/workflows/preview.yml Show resolved Hide resolved
@mehmetoguzderin
Copy link
Member Author

mehmetoguzderin commented Nov 4, 2020

Thanks for the feedback @plinss ! I did test building as such on my development repo, and it works fine! The latest push includes the change (I also created another PR to streamline that for the baseline action too, I wrote those lines based on that).

@pjoe
Copy link
Contributor

pjoe commented Nov 5, 2020

Just wondering: This would do the same build as was done in the build workflow? Could be more efficient to use that directly, but probably requires running on pull_request_target and using the trick mentioned earlier (https://github.com/bugout-dev/locust/blob/1bebcc2cac67e90c1efb09b42c37e91cdae829f9/.github/workflows/locust.yaml)

@mehmetoguzderin
Copy link
Member Author

Unless baseline Action is split for push and PR, I think we would need to rebuild. Though even then it would need additional testing, the current one is tested end to end.

@pjoe
Copy link
Contributor

pjoe commented Nov 5, 2020

Yeah, would require even more conditional logic.

Anyway just a possible optimization - let's get things working first :)

@kvark
Copy link
Contributor

kvark commented Nov 5, 2020

Ok, we have editors agreement here. And in the worst case, we'll be left with no previews. Let's move!

@kvark kvark merged commit b2792d3 into gpuweb:main Nov 5, 2020
@mehmetoguzderin mehmetoguzderin mentioned this pull request Nov 5, 2020
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
This PR adds tests for literal parsing of bool, int and float values.

Issue: gpuweb#1192
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.

None yet

6 participants