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

Test PR for preview #181

Closed
wants to merge 1 commit into from
Closed

Test PR for preview #181

wants to merge 1 commit into from

Conversation

netomi
Copy link
Contributor

@netomi netomi commented Sep 1, 2023

No description provided.

@netomi
Copy link
Contributor Author

netomi commented Sep 1, 2023

@spoenemann some test PR to check if the preview mechanism works. However as this PR comes from a fork the action to add a PR comment fails as PRs from forks dont get write tokens. There are workarounds for that, but for security reasons its best not to hand out write tokens to PRs from forks as everyone can create them.

I guess you will only have PRs from the repo itself?

@spoenemann spoenemann mentioned this pull request Sep 6, 2023
@spoenemann
Copy link
Contributor

I guess you will only have PRs from the repo itself?

There will often be contributions from forks. But we could change the repo settings to require approval by maintainers before running Actions?

@netomi
Copy link
Contributor Author

netomi commented Sep 6, 2023

The pr-preview-action action uses internally the sticky-pull-request-comment action which adds comments to the PR itself upon completion. This action needs write permissions for pull-requests. For PR from forks, GitHub does not issue write tokens during workflow runs for good reasons. There is a workaround that we also utilize in our validation scripts that can be found here: https://github.com/adoptium/.eclipsefdn/blob/main/.github/workflows/validate.yml

The idea is to use pull_request_target instead. You should read https://securitylab.github.com/research/github-actions-preventing-pwn-requests/ before using that approach.

@netomi
Copy link
Contributor Author

netomi commented Sep 6, 2023

Also, as you removed the netlify configuration from the repo, should the netlify app be uninstalled as it is not used anymore?

It had some advantages compared to deploying to gh-pages though imho.

@spoenemann
Copy link
Contributor

Yes we can remove the Netlify app. And yes, Netlify is more comfortable and offers very nice deploy previews, but we'd like to move the deployment to the Foundation. If you offer Netlify deployments as alternative to GH Pages, we can consider that as well.

@netomi
Copy link
Contributor Author

netomi commented Sep 6, 2023

The netlify app has been removed from the organization.

I am not aware of what the EF offers in terms of deployment environments for previews, best to raise a ticket in the HelpDesk.

@netomi
Copy link
Contributor Author

netomi commented Sep 7, 2023

btw the link to the issue in the deploy preview workflow from forks is here: rossjrw/pr-preview-action#3

@spoenemann
Copy link
Contributor

No solution in sight so far. I propose to remove the previews (#208) until someone comes up with a properly working preview feature.

@spoenemann
Copy link
Contributor

@netomi what's that bot doing with the PR?

@eclipse-langium-bot
Copy link
Contributor

I am trying something with the workflow and reopen the PR to trigger actions.

Copy link

PR Preview Action v1.4.4
🚀 Deployed preview to https://eclipse-langium.github.io/langium-website/pr-preview/pr-181/
on branch gh-pages at 2023-12-12 15:26 UTC

@eclipse-langium-bot
Copy link
Contributor

ok so the preview works and is also committed correctly to the gh-pages branch, however, there seems to be some redirect active that prevents that to be visible.

@eclipse-langium-bot
Copy link
Contributor

ok, so the normal site is deployed using GitHub actions, and the preview feature uses the gh-pages branch. The 2 things are not compatible with each other. So I guess we have solution to combine the 2 approaches.

@eclipse-langium-bot
Copy link
Contributor

I reverted the changes, remove the gh-pages branch and will close this PR again.

@netomi netomi deleted the test-pr branch December 12, 2023 15:59
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.

3 participants