-
Notifications
You must be signed in to change notification settings - Fork 33
Allow limited public assembler builds on docs-builder #2379
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
base: main
Are you sure you want to change the base?
Conversation
src/services/Elastic.Documentation.Assembler/Building/AssemblerBuildService.cs
Fixed
Show fixed
Hide fixed
src/services/Elastic.Documentation.Assembler/Building/AssemblerBuilder.cs
Fixed
Show fixed
Hide fixed
src/services/Elastic.Documentation.Assembler/Building/AssemblerBuildService.cs
Fixed
Show fixed
Hide fixed
src/services/Elastic.Documentation.Assembler/Sourcing/RepositorySourcesFetcher.cs
Fixed
Show fixed
Hide fixed
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Mpdreamz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Some changes requested.
.github/workflows/preview-build.yml
Outdated
| env: | ||
| AWS_RETRY_MODE: standard | ||
| AWS_MAX_ATTEMPTS: 6 | ||
| ASSEMBLER_PREVIEW_PATH_PREFIX: ${{ github.repository }}/assembled-docs/${{ github.event.pull_request.number }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ASSEMBLER_PREVIEW_PATH_PREFIX: ${{ github.repository }}/assembled-docs/${{ github.event.pull_request.number }} | |
| ASSEMBLER_PREVIEW_PATH_PREFIX: ${{ github.repository }}/docs/${{ github.event.pull_request.number }} |
.github/workflows/preview-build.yml
Outdated
| log_url: `https://github.com/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`, | ||
| }) | ||
| - name: Create Assembler Deployment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lost a comment, lets move this to dedicated new workflow file. This shared one is tooo complex already and shouldn't trigger on labeled.
| name: assembler-preview | ||
|
|
||
| on: | ||
| workflow_dispatch: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs PR and label triggers
| default: false | ||
| required: false | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert this file completely (the whitespace clean up is good but lets not make it part of this PR).
| public async ValueTask<bool> FinishExportAsync(IDirectoryInfo outputFolder, Cancel ctx) | ||
| { | ||
| var outputDirectory = Path.Combine(outputFolder.FullName, "docs"); | ||
| var outputDirectory = outputFolder.FullName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We expose llm.zip on elastic.co so I think this needs to be put under docs again.
| doc.Add(root); | ||
|
|
||
| using var fileStream = fileSystem.File.Create(Path.Combine(outputFolder.ToString() ?? string.Empty, "docs", "sitemap.xml")); | ||
| using var fileStream = fileSystem.File.Create(fileSystem.Path.Combine(outputFolder.FullName, "sitemap.xml")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sitemap.xml has to live under docs/sitemap.xml
Problem Overview
Certain changes in docs-builder that are best viewed from an assembler build perspective can benefit from the collective review from writers, but it's not always straightforward to share specific links, show regressions, etc.
Allowing a limited assembler preview for docs-builder pull requests would allow for earlier and broader QA, and faster communication during feature development.
Proposed solution
This PR adds a new workflow to docs-builder that builds an assembler preview to a given PR #.
It contains the following limitations:
Other changes
llms.txt,sitemap.xmlandlink-index.snapshot.jsongeneration relied on hardcoded paths, which were incompatible with the previews introduced here. If there is a path prefix available, we now make use of it.