Real redirects support #1397
Real redirects support #1397
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for real redirects by generating a redirects.json artifact during build and updating CloudFront’s KeyValueStore during deployment.
- New package references for CloudFront and its KeyValueStore in the csproj.
- Enhancements in CLI commands: outputting the redirect artifact during build and adding a deploy command to update redirects in CloudFront.
- Integration of redirects output within the assembler builder and serialization support enhancements.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/tooling/docs-assembler/docs-assembler.csproj | Added package references for AWS CloudFront and CloudFrontKeyValueStore. |
| src/tooling/docs-assembler/Cli/RepositoryCommands.cs | Outputs the path of the generated redirects.json artifact to GitHub Actions. |
| src/tooling/docs-assembler/Cli/DeployCommands.cs | Introduces a new command to update redirects in CloudFront’s KeyValueStore using batch operations. |
| src/tooling/docs-assembler/Building/AssemblerBuilder.cs | Adds logic to output redirects.json and filter duplicate redirect entries. |
| src/Elastic.Documentation/Serialization/SourceGenerationContext.cs | Updates serialization context to support FrozenDictionary. |
| Directory.Packages.props | Specifies versions for new AWS SDK package dependencies. |
Comments suppressed due to low confidence (1)
src/tooling/docs-assembler/Building/AssemblerBuilder.cs:155
- [nitpick] Consider adding an inline comment explaining why redirects with equivalent keys and values (after trimming trailing slashes) are being filtered out, for clarity.
.ToFrozenDictionary();
| { | ||
| var uniqueRedirects = redirects | ||
| .Where(x => !x.Key.TrimEnd('/').Equals(x.Value.TrimEnd('/'))) | ||
| .ToFrozenDictionary(); |
There was a problem hiding this comment.
nit: FrozenDictionary primary usecase is for dictionaries that live throughout the lifetime of the application a simple ToDictionary() suffices here and is slighltly faster to construct. nano-optimization nit though.
There was a problem hiding this comment.
That way we can also rely on the regular dictionary serialization.
| /// <param name="environment">The environment to build</param> | ||
| /// <param name="redirectsFile">Path to the redirects mapping pre-generated by docs-assembler</param> | ||
| /// <param name="ctx"></param> | ||
| [Command("update-redirects")] |
There was a problem hiding this comment.
Out of curiousity do you know how long this takes?
| } | ||
| while (!string.IsNullOrEmpty(listKeysResponse.NextToken)); | ||
|
|
||
| var toPut = sourcedRedirects |
There was a problem hiding this comment.
Should we add some error if this happens to go over the limit?
If it goes over do we need N keyvalue stores?
| .Select(k => new DeleteKeyRequestListItem { Key = k }); | ||
|
|
||
| ConsoleApp.Log("Updating redirects in KVS"); | ||
| const int batchSize = 500; |
There was a problem hiding this comment.
Just out of curiousity, did you play with this to observe overall runtime of the command?
| ConsoleApp.Log("Updating redirects in KVS"); | ||
| const int batchSize = 500; | ||
|
|
||
| eTag = await ProcessBatchUpdatesAsync(kvsClient, kvsArn, eTag, toPut, batchSize, "Puts", ctx); |
There was a problem hiding this comment.
No magic strings Puts Deletes please!
| @@ -136,6 +136,10 @@ public async Task<int> BuildAll( | |||
|
|
|||
| await cloner.WriteLinkRegistrySnapshot(checkoutResult.LinkRegistrySnapshot, ctx); | |||
|
|
|||
| var redirectsPath = Path.Combine(assembleContext.OutputDirectory.FullName, "redirects.json"); | |||
| if (File.Exists(redirectsPath)) | |||
| await githubActionsService.SetOutputAsync("redirects_artifact_path", redirectsPath); | |||
There was a problem hiding this comment.
This will also need to be declared in the github action as output, will satisfy IDE checks nicely :)
| using Microsoft.Extensions.Logging; | ||
| using DescribeKeyValueStoreRequest = Amazon.CloudFront.Model.DescribeKeyValueStoreRequest; |
There was a problem hiding this comment.
super nit: why the type alias here?
| private async Task OutputRedirectsAsync(Dictionary<string, string> redirects, Cancel ctx) | ||
| { | ||
| var uniqueRedirects = redirects | ||
| .Where(x => !x.Key.TrimEnd('/').Equals(x.Value.TrimEnd('/'))) |
There was a problem hiding this comment.
Can we ensure CollectRedirects never adds these self referential redirects?
This MR is part of https://github.com/elastic/docs-eng-team/issues/135.
In order to have "real redirects", we need to provide a redirect mapping to CloudFront's KeyValueStore. CloudFront in turn will use it as it handles requests, determining passthrough or redirection.
docs-assembler repo build-allnow outputs a redirects.json file with the compiled mappings. It outputs the path to this file to Github's CI underredirects_artifact_path.docs-assemblerreceived a new command:deploy update-redirects. It will be called on CI during deployment, access the redirects mapping artifact fromrepo build-all, access the KeyValueStore, determine which redirects should be added, kept or deleted, and perform batch operations.