CI: Support PR Preview #38

Shaikh-Ubaid · 2022-08-21T22:22:04Z

This PR adds support of previewing a pull request. The built site is currently pushed into gh-pages branch at https://github.com/Shaikh-Ubaid/pull_request_preview.

Example PR: https://github.com/Shaikh-Ubaid/lcompilers_frontend/pull/2

Update links in pr_preview workflow CI: Merge steps checkout, commit and deploy CI: cd into pr_preview CI: Use global user CI: Use deploy_repo_push link to deploy CI: Push to main branch CI: Define a separate upload_site.sh file

Shaikh-Ubaid · 2022-08-21T22:30:29Z

It seems the Create comment about deployment step failed. I guess the GitHub actions bot is unable to comment as the CI is not yet a part of master or because the CI might not be running in its own GitHub remote. Since, the comment works at https://github.com/Shaikh-Ubaid/lcompilers_frontend/pull/2, I am hoping it would work at https://github.com/lfortran/lcompilers_frontend as well (I guess the pull request might need to be created at/from origin/feature_branch).

Shaikh-Ubaid · 2022-08-21T22:30:51Z

This is ready. Please possibly review and please share feedback.

certik · 2022-08-22T03:26:30Z

I would reuse the workflow that we have: the main purpose of the preview build is to ensure that it will work in production. For that we need to use the exact same steps, including checking out any production repositories etc. Only at the very end, instead of pushing into production, we push into the test repository.

Shaikh-Ubaid · 2022-08-22T09:04:32Z

This is ready. Please possibly review and please share feedback.

Shaikh-Ubaid · 2022-08-22T09:14:29Z

I transferred the pull_request_preview repository here https://github.com/lfortran/pull_request_preview

certik · 2022-08-22T09:15:45Z

ci/upload_site.sh

+if [[ ${git_ref} == "refs/heads/main" ]]; then
+    # Production version - pipeline triggered from main branch
+    deploy_repo_pull="https://github.com/lfortran/lcompilers_frontend.git"
+    deploy_repo_push="[email protected]:lfortran/lcompilers_frontend.git"
+else
+    # Test version - pipeline triggered from pull request
+    deploy_repo_pull="https://github.com/Shaikh-Ubaid/pull_request_preview.git"
+    deploy_repo_push="[email protected]:Shaikh-Ubaid/pull_request_preview.git"
+fi


Why not:

Suggested change

if [[ ${git_ref} == "refs/heads/main" ]]; then

# Production version - pipeline triggered from main branch

deploy_repo_pull="https://github.com/lfortran/lcompilers_frontend.git"

deploy_repo_push="[email protected]:lfortran/lcompilers_frontend.git"

else

# Test version - pipeline triggered from pull request

deploy_repo_pull="https://github.com/Shaikh-Ubaid/pull_request_preview.git"

deploy_repo_push="[email protected]:Shaikh-Ubaid/pull_request_preview.git"

fi

deploy_repo_pull="https://github.com/lfortran/lcompilers_frontend.git"

if [[ ${git_ref} == "refs/heads/main" ]]; then

# Production version - pipeline triggered from main branch

deploy_repo_push="[email protected]:lfortran/lcompilers_frontend.git"

else

# Test version - pipeline triggered from pull request

deploy_repo_push="[email protected]:Shaikh-Ubaid/pull_request_preview.git"

fi

doubt: Please, could you possibly share if the above approach could lead to errors such as:

(Not an exact error, just an example)

Failed to push refs: The repo is x commits ahead, use git pull

If we pull the pull_request_preview and then update/push to it, I guess, we are (hopefully) avoiding or reducing the chances of Failed to push refs error.

Good point. I think in the past I did force push, but force pushing to a production repository might not be a good practice, since it can hide issues.

How are different PRs handled, or do all PRs push into just master, so they overwrite each other?

How are different PRs handled, or do all PRs push into just master, so they overwrite each other?

Yes, it seems they would overwrite each other.

This reverts commit 0e80d77.

Shaikh-Ubaid · 2022-08-22T09:29:57Z

I updated the links to point to the new repository at https://github.com/lfortran/pull_request_preview

Shaikh-Ubaid · 2022-08-22T09:30:55Z

Ondrej Sir, please, could you possibly share if it would be possible for you to setup the needed SSH Keys, please?

Shaikh-Ubaid · 2022-08-22T09:31:57Z

Also, I updated the preview link here 893812e to https://preview.dev.lfortran.org. Please, could someone possibly share if the link is fine and if it is possible to use it?

Shaikh-Ubaid · 2022-08-22T09:35:28Z

Also, I updated the preview link here 893812e to https://preview.dev.lfortran.org/. Please, could someone possibly share if the link is fine and if it is possible to use it?

If we use a link which has some path after domain (for example: https://dev.lfortran.org/pull_request_preview), we would be needing to update the basepath as in here 0e80d77. It seems updating the basepath might be inconsistent with the production version which is deployed to https://dev.lfortran.org/ (which has no basepath).

Shaikh-Ubaid · 2022-08-22T09:37:23Z

This is ready. Please possibly review and please share feedback.

certik · 2022-08-22T10:39:54Z

.github/workflows/deploy.yml

+      with:
+        issue-number: ${{ github.event.pull_request.number }}
+        body: |
+          Your site is deployed at https://preview.dev.lfortran.org/


I don't know how subdomains work --- we can also just use regular github pages, to make it clear it is just a PR preview.

I don't know how subdomains work --- we can also just use regular github pages, to make it clear it is just a PR preview.

It seems when we use the regular github pages, the site would be available at https://lfortran.github.io/pull_request_preview and thus adding the path /pull_request_preview to the url. Supporting the path might be challenging as I shared here #38 (comment)

certik · 2022-08-22T10:41:17Z

ci/upload_site.sh

+    echo "Note: GIT_PR_PREVIEW_PRIVATE_SSH_KEY is empty, skipping..."
+    exit 0
+fi
+ssh-add <(echo "$GIT_PR_PREVIEW_PRIVATE_SSH_KEY" | base64 -d)


We already have ssh keys setup with a different variable (I believe), so this PR will only use GIT_PR_PREVIEW_PRIVATE_SSH_KEY to push even to production?

I think we should use the production keys for production and preview keys for previews, what do you think?

We already have ssh keys setup with a different variable (I believe), so this PR will only use GIT_PR_PREVIEW_PRIVATE_SSH_KEY to push even to production?

It seems there are no ssh keys required to push to production (as we discussed here #1 (comment) and also tried/tested it by removing the SSH key setup step here c6cee87). We currently need the SSH keys for pushing the test version to pull_request_preview (as pull_request_preview is a separate repository). During production deployment, even if the SSH keys would be available, I guess/think that they would (hopefully) not be used.

We already have ssh keys setup with a different variable (I believe)

Please, could you possibly share if it would be possible to rename it and setup its public key at https://github.com/lfortran/pull_request_preview, please

I see. It's a little weird that it would quit to push to production if GIT_PR_PREVIEW_PRIVATE_SSH_KEY is empty. But I guess that's fine. Would it make sense to put this into if [[ ${git_ref} != "refs/heads/main" ]]; then if statement?

Yes, got it.

certik · 2022-08-22T12:18:26Z

I setup the keys.

I think we should do it to serve in a new branch, such as pr-38 for this PR. That way we can have more than one open PR and it will not clash.

Regarding the prefix, so that it does not clash, I think that's fine, if it can be done in some clean (hard to break) way. Usually if it works with a prefix, hopefully it will work with an empty prefix too.

Shaikh-Ubaid · 2022-08-22T12:19:29Z

I think we should do it to serve in a new branch, such as pr-38 for this PR. That way we can have more than one open PR and it will not clash.

It seems GitHub Pages allow setting only a single branch for deployment. So, it seems everytime we might need to change the branch of deployment.

certik · 2022-08-22T13:53:37Z

The way to do it is to use a prefix directory with the PR number.

Shaikh-Ubaid · 2022-08-22T18:23:44Z

The way to do it is to use a prefix directory with the PR number.

Got it. Thank you so much for the guidance.

Shaikh-Ubaid · 2022-08-22T18:25:46Z

I came across nf-core/tools#765 and https://securitylab.github.com/research/github-actions-preventing-pwn-requests/. It seems to me that we can support previewing pull requests even when the PRs are submitted from a fork.

Shaikh-Ubaid · 2022-08-22T18:33:00Z

I came across nf-core/tools#765 and https://securitylab.github.com/research/github-actions-preventing-pwn-requests/. It seems to me that we can support previewing pull requests even when the PRs are submitted from a fork.

I would like to try this out. Please, could you possibly share if we shall create three separate workflows as follows:

first workflow runs only on push to main branch event
- It is the same one that we currently have in the master
second workflow runs only on pull request event
- It runs in an unprivileged environment and builds the project and uploads the built files
third workflow runs only on after completion of the second workflow
- It runs in a privileged environment
- It downloads the built files of the second workflow and pushes them to the pull_request_preview repository
- It comments on the pull request with the deployment link

Notes:

unprivileged environment means the workflow does not have access to secrets/tokens.
privileged environment means the workflow has access to secrets/tokens.

ci/upload_site.sh

Co-authored-by: Ondřej Čertík <[email protected]>

certik

I think this is fine. When you merge, carefully watch the builds, and test the final site. If something goes wrong, then first revert the deploy commit in the deploy repo to fix the site to the previous state, then try to fix it with new PRs.

Shaikh-Ubaid · 2022-08-22T19:47:21Z

The way to do it is to use a prefix directory with the PR number.

Ondrej Sir, it seems, the workflow currently overwrites the gh-pages branch for every change in different pull-requests.

Shaikh-Ubaid · 2022-09-09T15:02:05Z

I think this is fine. When you merge, carefully watch the builds, and test the final site. If something goes wrong, then first revert the deploy commit in the deploy repo to fix the site to the previous state, then try to fix it with new PRs.

Got it.

I looked into the changes of this pull request again and they seem fine to me. Since, this seems to be approved, I am merging this PR for now.

Shaikh-Ubaid · 2022-09-09T15:12:35Z

The CI failed to push to gh-pages branch of https://github.com/lfortran/lcompilers_frontend. It seems that since we used the https url to clone the repository https://github.com/lfortran/lcompilers_frontend, it is needing ssh keys to push to the same repository.

I am submitting a new PR which would use the ssh url for the same repository https://github.com/lfortran/lcompilers_frontend for the commit in the main branch.

Since, the push to gh-pages branch of https://github.com/lfortran/lcompilers_frontend failed, the site should be hopefully intact. It also seems to be.

Shaikh-Ubaid · 2022-09-09T22:56:05Z

I am submitting a new PR which would use the ssh url for the same repository https://github.com/lfortran/lcompilers_frontend for the commit in the main branch.

I fixed the CI failure issue while pushing to gh-pages branch of https://github.com/lfortran/lcompilers_frontend.

Also, while updating the latest_commit, I fixed the issues discovered related to this pull_request_preview feature in #44.

certik · 2022-09-11T15:39:19Z

Thanks for fixing it!

CI: Add pull request preview workflow

95dee33

Update links in pr_preview workflow CI: Merge steps checkout, commit and deploy CI: cd into pr_preview CI: Use global user CI: Use deploy_repo_push link to deploy CI: Push to main branch CI: Define a separate upload_site.sh file

Shaikh-Ubaid requested a review from certik August 21, 2022 22:22

Shaikh-Ubaid force-pushed the ci_pr_preview branch 3 times, most recently from 35d4e79 to 52e70e3 Compare August 22, 2022 09:01

Shaikh-Ubaid marked this pull request as draft August 22, 2022 09:13

certik reviewed Aug 22, 2022

View reviewed changes

Shaikh-Ubaid added 9 commits August 22, 2022 14:48

CI: Add steps from pr_preview.yml to deploy.yml

a2faec7

CI: Remove pr_preview.yml workflow

cdd3d6c

CI: Run deploy workflow on pull requests

d332be8

CI: Push to respective repos depending on branch

96b256c

CI: Comment only on pull request and when running at origin

e821a33

CI: Update the repository links

f7622ba

CI: Update preview link

893812e

Fix path

0e80d77

Revert "Fix path"

e881b20

This reverts commit 0e80d77.

Shaikh-Ubaid force-pushed the ci_pr_preview branch from 52e70e3 to e881b20 Compare August 22, 2022 09:19

Shaikh-Ubaid marked this pull request as ready for review August 22, 2022 09:37

certik reviewed Aug 22, 2022

View reviewed changes

CI: Fix: pushing to production

292a1f9

Shaikh-Ubaid force-pushed the ci_pr_preview branch from 9d318dc to 292a1f9 Compare August 22, 2022 12:18

Shaikh-Ubaid marked this pull request as draft August 22, 2022 18:37

certik reviewed Aug 22, 2022

View reviewed changes

ci/upload_site.sh Outdated Show resolved Hide resolved

Do not use ssh keys when in main branch

3e51eb2

Co-authored-by: Ondřej Čertík <[email protected]>

certik approved these changes Aug 22, 2022

View reviewed changes

Shaikh-Ubaid marked this pull request as ready for review September 9, 2022 15:02

Shaikh-Ubaid merged commit 5a977ed into lfortran:main Sep 9, 2022

This was referenced Sep 9, 2022

CI: Fix pushing to gh-pages on commit to main #41

Merged

CI: Fix #42

Merged

CI: Fix: Use origin as deploy_push url for commits to main #43

Merged

Shaikh-Ubaid deleted the ci_pr_preview branch September 9, 2022 16:32

Shaikh-Ubaid mentioned this pull request Sep 9, 2022

Update lfortran commit sha #44

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI: Support PR Preview #38

CI: Support PR Preview #38

Shaikh-Ubaid commented Aug 21, 2022

Shaikh-Ubaid commented Aug 21, 2022

Shaikh-Ubaid commented Aug 21, 2022

certik commented Aug 22, 2022

Shaikh-Ubaid commented Aug 22, 2022

Shaikh-Ubaid commented Aug 22, 2022

certik Aug 22, 2022

Shaikh-Ubaid Aug 22, 2022

Shaikh-Ubaid Aug 22, 2022

certik Aug 22, 2022

Shaikh-Ubaid Aug 22, 2022

Shaikh-Ubaid commented Aug 22, 2022

Shaikh-Ubaid commented Aug 22, 2022

Shaikh-Ubaid commented Aug 22, 2022 •

edited

Loading

Shaikh-Ubaid commented Aug 22, 2022 •

edited

Loading

Shaikh-Ubaid commented Aug 22, 2022

certik Aug 22, 2022

Shaikh-Ubaid Aug 22, 2022

certik Aug 22, 2022

Shaikh-Ubaid Aug 22, 2022

Shaikh-Ubaid Aug 22, 2022

certik Aug 22, 2022

Shaikh-Ubaid Aug 22, 2022

certik commented Aug 22, 2022

Shaikh-Ubaid commented Aug 22, 2022 •

edited

Loading

certik commented Aug 22, 2022

Shaikh-Ubaid commented Aug 22, 2022

Shaikh-Ubaid commented Aug 22, 2022

Shaikh-Ubaid commented Aug 22, 2022 •

edited

Loading

certik left a comment

Shaikh-Ubaid commented Aug 22, 2022

Shaikh-Ubaid commented Sep 9, 2022

Shaikh-Ubaid commented Sep 9, 2022

Shaikh-Ubaid commented Sep 9, 2022

certik commented Sep 11, 2022

CI: Support PR Preview #38

CI: Support PR Preview #38

Conversation

Shaikh-Ubaid commented Aug 21, 2022

Shaikh-Ubaid commented Aug 21, 2022

Shaikh-Ubaid commented Aug 21, 2022

certik commented Aug 22, 2022

Shaikh-Ubaid commented Aug 22, 2022

Shaikh-Ubaid commented Aug 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shaikh-Ubaid commented Aug 22, 2022

Shaikh-Ubaid commented Aug 22, 2022

Shaikh-Ubaid commented Aug 22, 2022 • edited Loading

Shaikh-Ubaid commented Aug 22, 2022 • edited Loading

Shaikh-Ubaid commented Aug 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

certik commented Aug 22, 2022

Shaikh-Ubaid commented Aug 22, 2022 • edited Loading

certik commented Aug 22, 2022

Shaikh-Ubaid commented Aug 22, 2022

Shaikh-Ubaid commented Aug 22, 2022

Shaikh-Ubaid commented Aug 22, 2022 • edited Loading

certik left a comment

Choose a reason for hiding this comment

Shaikh-Ubaid commented Aug 22, 2022

Shaikh-Ubaid commented Sep 9, 2022

Shaikh-Ubaid commented Sep 9, 2022

Shaikh-Ubaid commented Sep 9, 2022

certik commented Sep 11, 2022

Shaikh-Ubaid commented Aug 22, 2022 •

edited

Loading

Shaikh-Ubaid commented Aug 22, 2022 •

edited

Loading

Shaikh-Ubaid commented Aug 22, 2022 •

edited

Loading

Shaikh-Ubaid commented Aug 22, 2022 •

edited

Loading