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

Added Feature to deploy previews of pull requests #638

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

devanshkansagra
Copy link
Contributor

Pull Request Preview Deployment to GitHub Pages

Acceptance Criteria fulfillment

  • Deploy every pull request builds of monorepo to a unique url
  • Remove Deployment when PR is merged and closed

Video/Screenshots

image

types:
- completed

permissions:
Copy link
Collaborator

@Spiral-Memory Spiral-Memory Oct 5, 2024

Choose a reason for hiding this comment

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

Once check if can remove any permission from here (as this workflow will have write permission)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we will need these permissions for deploying builds to github-pages. permissions:content: write

Copy link
Collaborator

Choose a reason for hiding this comment

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

pages: write
id-token: write

Are these necessary ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pages write gives permissions to deploy from github pages and id-token I don't know, it was there in build-lint so I used it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Try removing permissions one by one and test if it is still working fine, let's allow as less permissions as we can

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

pull_request_review:
types: submitted

permissions:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same (but here it is less risky)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed and working well

.github/workflows/ci-pr.yml Outdated Show resolved Hide resolved

env:
LAYOUT_EDITOR_BASE_URL: "/EmbeddedChat/pulls/pr-${{github.event.pull_request.number}}/layout_editor"
DOCS_BASEURL: "/EmbeddedChat/pulls/pr-${{github.event.pull_request.number}}/docs"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use DOCS_BASE_URL for consistency (also make sure the application is expecting the same env variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

build:
if: github.event.review.state == 'approved' && (github.event.review.author_association == 'COLLABORATOR' || github.event.review.author_association == 'OWNER')
runs-on: ubuntu-latest
outputs:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove outputs as discussed and tested (as it has no use)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

with:
ref: gh-deploy

- name: Remove Deployment
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make sure it must not run if the PR is closed / merged before it got apporved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -0,0 +1,37 @@
name: Deploy PR-Preview
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's rename cd-pr.yml as deploy-pr-preview.yml and ci as build-pr-preview.yml

CI/CD name is confusing as other workflow is also a part of CI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed the name

@Spiral-Memory Spiral-Memory added the epic this is a must have feature label Oct 5, 2024
Copy link
Collaborator

@Spiral-Memory Spiral-Memory left a comment

Choose a reason for hiding this comment

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

Approving to test build !

@Spiral-Memory Spiral-Memory marked this pull request as draft October 5, 2024 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
epic this is a must have feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants