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

Add comment to PR with preview link #87

Merged
merged 1 commit into from
May 19, 2023

Conversation

holly-cummins
Copy link
Contributor

I noticed we're not adding comments to PRs, even though we do the hard work of making a preview.

@holly-cummins
Copy link
Contributor Author

Sadly, the version of the workflow run in the PR is the one in main, not in this PR, so I'm not sure we can test it without merging.

@gastaldi
Copy link
Member

This is weird. Can you check if the comment is created for a PR coming from a branch from this repository (not from a fork?).

AFAIR the surge-action already does all the work, so if we're using it I don't think it needs extra steps

@holly-cummins
Copy link
Contributor Author

holly-cummins commented May 19, 2023

I agree that the docs show pictures of comments, which makes it look like the action does everything. In https://github.com/quarkusio/quarkusio.github.io/blob/develop/.github/workflows/preview.yml we have to add the comments manually, but we're not using the action, just a npx command, so that makes sense.

Unfortunately, I don't think I have permissions to do a PR from a branch of this repo. I agree it's a good experiment to try.

Oh wait, I think I see the issue. The docs have this:

    permissions:
     pull-requests: write # allow surge-preview to create/update PR comments

I'll try adding that instead.

@gastaldi
Copy link
Member

Let's see how that goes

@gastaldi gastaldi merged commit 9ca9a37 into quarkiverse:main May 19, 2023
@holly-cummins
Copy link
Contributor Author

Argh, thanks for the merge - but I think we're not there yet. :(

 PR created from a forked repository, so skip PR comment

I checked back through the other builds of the earlier version of this PR, and it's the same message. So I don't even know what the permissions flag does. More work needed, since most PRs will be from forks.

@gastaldi
Copy link
Member

since most PRs will be from forks

I don't think so, as members of the quarkiverse-members team are allowed to push and create branches here

@holly-cummins
Copy link
Contributor Author

since most PRs will be from forks

I don't think so, as members of the quarkiverse-members team are allowed to push and create branches here

Oh, that would help!

I just did a quick survey of the recent PRs to check.

So the first one I checked was from a fork, but all the others were from the main repo. I guess it's just Peter and I who hadn't got the memo. And my change didn't really do anything. :)

Scrolling down further I can find a few others from forks, like #63 and #64, but not so many. So I guess that's ok.

@holly-cummins
Copy link
Contributor Author

(Another of way of checking, I guess, is to see that we have 31 forks of this repo, and 89 PRs. So if every fork did exactly one PR, that would be a third of the PRs originating from a fork ... but some of those forks probably didn't originate any PRs.)

@gastaldi
Copy link
Member

I wonder if splitting the CI preview as proposed in #44 would help

@holly-cummins
Copy link
Contributor Author

I think so. The main quarkus docs site, and extensions.quarkus.io, have previews from forks, using a similar technique, I think.

While I was reading the surge docs I also noticed that we should be able to have just one surge workflow. If we trigger on both open and close, and set teardown: true, it seems like surge will figure out whether to standup or teardown.

@holly-cummins holly-cummins deleted the preview-ci-comment branch May 19, 2023 18:15
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