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

Improve pull request previews #852

Closed
dgarcia360 opened this issue Aug 8, 2023 · 8 comments
Closed

Improve pull request previews #852

dgarcia360 opened this issue Aug 8, 2023 · 8 comments
Assignees

Comments

@dgarcia360
Copy link
Collaborator

dgarcia360 commented Aug 8, 2023

Amplify triggers on every build, not only if there are pull request related changes.

We are evaluating to push directly to Amplify / S3 Bucket / PushPreview the artifact generated with the "Docs / PR" workflow.

@benipeled
Copy link

We should raise the priority for this item - the emails about amplify comments that are later deleted is annoying,
The plan is to use two workflow

  1. Create (workflow already exists) - trigger for pull request events: opened, synchronize, reopened
    1.1 Build
    1.2 Test
    1.3 Publish to a dedicated folder on an S3 bucket, ex. s3://<URL>/<PR_NUMBER>
    1.4 Post a comment with a URL to the built docs

  2. Delete - trigger for pull request events: closed
    2.1 Delete doc PR folder from the S3 bucket

If no one else started implementing something - I'll do

PS> We can avoid the second workflow by setting S3 lifecycle to auto-delete content > 90days (and mentioned it on the PR comment) it will save us a few $ on github-actions bill

@tzach
Copy link
Collaborator

tzach commented Aug 14, 2023

The plan make sense to me, I will be happy to see a working demo.

PS> We can avoid the second workflow by setting S3 lifecycle to auto-delete content > 90days (and mentioned it on the PR comment) it will save us a few $ on github-actions bill

Less worries about the cost, and more about old, out dated preview floating around, maybe indexed by Google.

@benipeled
Copy link

The plan make sense to me, I will be happy to see a working demo.

PS> We can avoid the second workflow by setting S3 lifecycle to auto-delete content > 90days (and mentioned it on the PR comment) it will save us a few $ on github-actions bill

Less worries about the cost, and more about old, out dated preview floating around, maybe indexed by Google.

No worry about google (and other search engines) - should be blocked by robots.txt

@dgarcia360 dgarcia360 self-assigned this Aug 14, 2023
@tzach tzach assigned benipeled and unassigned dgarcia360 Aug 14, 2023
@tzach
Copy link
Collaborator

tzach commented Aug 14, 2023

@dgarcia360 I understand @benipeled already has a close-to-ready solution, so I need to work on this for now.
I would appreciate your help reviewing the propose solution (PR)

@dgarcia360
Copy link
Collaborator Author

Hi @benipeled,

The solution outlined will only work for users who are invited to the repo and have write permissions. Note that contributors submitting pull requests from forks will not be able to access the encrypted variable that will contain the S3 key.

Given this, I recommend triggering the docs build only when a label is added to a pull request by a maintainer or someone with direct write access, rather than on every push.

I have availability too if you need anything from my side.

@benipeled
Copy link

Thanks @dgarcia360 for the input I'll check this option

benipeled added a commit to benipeled/scylla that referenced this issue Aug 27, 2023
The Amplify PR preview recently added has no option to run against docs
changes only but it triggers against all pull-requests and create a comment
that is later deleted (part of the amplify-enhanced flow) - this flow creates
too much noise

This change removes the Amplify workflow and implements the preview solution
as part of the existing github-action workflow, The workflow:

- Triggers for all doc changes, not just master/enterprise
- Docs preview will be published to S3 and hosted on a based-PR URL dev-docs.scylladb.com/repo-name/pr-number
- A PR comment will be posted with direct links to changed pages.
- The preview content will be deleted from S3 once the PR will be merged/closed

Ref: scylladb/sphinx-scylladb-theme#852 (comment)
@benipeled
Copy link

The docs-preview implemented by a new Jenkins pipeline (part of the pkg repo)
@dgarcia360 I have no permissions to close this issue - please close it

@benipeled
Copy link

The docs-preview implemented by a new Jenkins pipeline (part of the pkg repo) @dgarcia360 I have no permissions to close this issue - please close it

@annastuchlik / @tzach please close this issue

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

No branches or pull requests

3 participants