search

gis-ops / valhalla-app

This is the demo web app running on https://valhalla.openstreetmap.de
https://valhalla.openstreetmap.de
MIT License
153 stars 87 forks source link

ci: trigger preview for PR #184

Closed Ananya2001-an closed 1 year ago

Ananya2001-an commented 1 year ago

👨‍💻 Changes proposed

Triggers a preview when a PR is raised.

đź“„ Note to reviewers

Ready for review

Ananya2001-an commented 1 year ago

Hey @nilsnolde I have created a draft PR for feedback purpose.

Ananya2001-an commented 1 year ago

I have not made all changes yet so don't test till I tell u to.

Ananya2001-an commented 1 year ago

It's working now you can see this preview for the PR on my fork which changes the title of the app: https://valhalla-app-tests.gis-ops.com/3/

Ananya2001-an commented 1 year ago

Now I just have to add a step for deletion of the folder on PR close. Also I forgot to mention earlier but we have to update that permalink function as well....

nilsnolde commented 1 year ago

Yeah, works great, thanks!

Also I forgot to mention earlier but we have to update that permalink function as well....

It's not essential for this to work, but it's still weird code yeah.

Ananya2001-an commented 1 year ago

It's not essential for this to work, but it's still weird code yeah.

Actually we don't need to update it. Because it works fine. Only problem is when the page reloads the path would be gone.

Ananya2001-an commented 1 year ago

Everything is done now:

We just need to create the necessary vars and secrets. Also for the last step Preview URL we would need a Personal access token to use github API and make the POST request. I have changed the variable and secret names since this host server is different from the original one. Hope everything is good :)

Also u can see this PR on my fork to see the kind of comment we are getting on successful deployment: https://github.com/Ananya2001-an/valhalla-app/pull/4

nilsnolde commented 1 year ago

Also u can see this PR on my fork to see the kind of comment we are getting on successful deployment: https://github.com/Ananya2001-an/valhalla-app/pull/4

sorry, just reading your comment. huh, strange, I was wondering how would the preview comment work if there's .issue, not .pull_request referenced in the API request. but then this does create a comment in this PR, not in an issue, right? weird..

I created an access token for this and registered all vars & secrets. at least I think I did the right thing with the access token, it became pretty confusing since the last time I did that..

I wonder why this workflow wasn't triggered on your last push? it was a synchronize event and checked all the other boxes no?

nilsnolde commented 1 year ago

I wonder why this workflow wasn't triggered on your last push? it was a synchronize event and checked all the other boxes no?

ah it didn't because it's not part of the files it's watching out for. we should probably include the respective workflow file as well: we'll want to run the workflow when we change smth.

nilsnolde commented 1 year ago

One other thing we could add: running the linters before building & deploying the app. I'm not sure what we use in this repo tbh, at least prettier I guess? others?

IMO best to include that as command in package.json so we can just call it with npm.

nilsnolde commented 1 year ago

apparently eslint as well? probably that also needs some == fixes and similar.. not sure how sound our eslint config is though..

nilsnolde commented 1 year ago

actually I'd almost prefer to run this only on workflow_dispatch. I can imagine how this clutters the PR interface terribly, if there's one comment for each commit. but even for manual dispatches it's still potentially very noisy, and worse, without any added value, the URL will never change for a PR.

hmm.. since there's a script running, one could in theory check all comments on this PR and there's any comment by the user gisops-bot we don't create an issue? if there's any comment by that user on a PR, we know that we already have the comment with the URL. opinions?

nilsnolde commented 1 year ago

sorry for being so nitpicky, but this is a really good action which we'll use in many other (at least private) projects I think.

Ananya2001-an commented 1 year ago

sorry, just reading your comment. huh, strange, I was wondering how would the preview comment work if there's .issue, not .pull_request referenced in the API request. but then this does create a comment in this PR, not in an issue, right? weird..

Actually every PR is also an issue in GitHub and what actually matters is the number so this same api would work to create a comment inside the issue as well as a PR.

I created an access token for this and registered all vars & secrets. at least I think I did the right thing with the access token, it became pretty confusing since the last time I did that..

Great 👍🏼

Ananya2001-an commented 1 year ago

hmm.. since there's a script running, one could in theory check all comments on this PR and there's any comment by the user gisops-bot we don't create an issue? if there's any comment by that user on a PR, we know that we already have the comment with the URL. opinions?

Yeah that's what I would look into

Ananya2001-an commented 1 year ago

sorry for being so nitpicky, but this is a really good action which we'll use in many other (at least private) projects I think.

No worries.. I appreciate detailed feedback and definitely this will be a good action to use everywhere else so we need to make sure that it's done right the first time :)

Ananya2001-an commented 1 year ago

So let me summarize the changes I need to make right now:

I hope that's all for now?

nilsnolde commented 1 year ago

Jep just one more tiny thing: we’ll also need to add the PR workflow file to the list of files triggering a build. That’ll also tell if it worked before merging.

You’re right, workflow_dispatch wouldn’t get the PR, only the branch. We could do smth more elaborate with the script and GitHub API but it’s not worth the effort.

Ananya2001-an commented 1 year ago

Jep just one more tiny thing: we’ll also need to add the PR workflow file to the list of files triggering a build. That’ll also tell if it worked before merging.

Yeah u are right we will do that.

You’re right, workflow_dispatch wouldn’t get the PR, only the branch. We could do smth more elaborate with the script and GitHub API but it’s not worth the effort.

Exactly 👍🏼

Ananya2001-an commented 1 year ago

Hey @nils You have to edit the scopes for PAT. It should have the repo scope. Is this token fine grained or classic because I used a classic one since setting scopes was easy there. I couldn't understand that for fine grained.

nilsnolde commented 1 year ago

Jep, I'll look at it in a bit

gisops-bot commented 1 year ago

Preview is ready! 🚀 You can view it here: https://valhalla-app-tests.gis-ops.com/184

nilsnolde commented 1 year ago

Woohoo, it worked! This is great @Ananya2001-an :) Seems like the only thing we need to watch out for, is that we don't immediately merge after triggering a build. I'd imagine that'd first remove the folder on our server and then re-create it. Small price to pay and happens rarely.