Skip to content

Conversation

Mpdreamz
Copy link
Member

@Mpdreamz Mpdreamz commented Mar 13, 2025

After #732 landed we now publish links.json files for cloud but on master.

https://elastic-docs-link-index.s3.us-east-2.amazonaws.com/elastic/cloud/master/links.json

This PR removes hardcoded paths to /main/links.json and uses paths as advertised by the links registry.

https://elastic-docs-link-index.s3.us-east-2.amazonaws.com/link-index.json

"cloud": {
      "master": {
        "repository": "cloud",
        "path": "elastic/cloud/master/links.json",
        "branch": "master",
        "etag": "64f49248eb9d421065125c346b4d0ce4"
      }
    },

@Copilot Copilot AI review requested due to automatic review settings March 13, 2025 16:26
@Mpdreamz Mpdreamz requested a review from a team as a code owner March 13, 2025 16:26
@Mpdreamz Mpdreamz added the fix label Mar 13, 2025
@Mpdreamz Mpdreamz self-assigned this Mar 13, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request removes hardcoded link references and instead relies on advertised paths obtained from a links registry. Key changes include:

  • Updating method signatures in CrossLinkResolver to accept FetchedCrossLinks for consistent link lookup.
  • Changing the GetBranch method in IUriEnvironmentResolver from private to public and adding a new branch mapping for the "cloud" scheme.
  • Refactoring and extending fetcher and configuration classes to support the new LinkIndexEntries property and to cover the newly added "cloud" repository.

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/Elastic.Markdown/CrossLinks/CrossLinkResolver.cs Updated TryFullyValidate and LookupLink to include fetchedCrossLinks for link resolution.
src/Elastic.Markdown/CrossLinks/IUriEnvironmentResolver.cs Exposed GetBranch as public and added a new mapping for "cloud" to "master".
docs/testing/cross-links.md Added a new example for a "cloud" link reference.
tests/Elastic.Markdown.Tests/TestCrossLinkResolver.cs Updated tests to incorporate new LinkIndexEntries in the fetched cross links.
src/Elastic.Markdown/CrossLinks/CrossLinkFetcher.cs Modified link index fetching logic to prioritize both "main" and "master" branch scenarios.
src/Elastic.Markdown/InboundLinks/LinkIndexLinkChecker.cs Adjusted link index URL construction to use LinkIndexEntries when available.
docs/_docset.yml Added the "cloud" repository to the cross_links configuration.
src/Elastic.Markdown/InboundLinks/LinkIndexCrossLinkFetcher.cs, ConfigurationCrossLinkFetcher.cs, AssemblerCrossLinkFetcher.cs Refactored variable names and added retrieval of LinkIndexEntries for consistency.
Comments suppressed due to low confidence (3)

src/Elastic.Markdown/CrossLinks/CrossLinkResolver.cs:114

  • [nitpick] Consider extracting the lookupPath computation into a dedicated helper method to reduce duplication and improve clarity.
var lookupPath = (crossLinkUri.Host + '/' + crossLinkUri.AbsolutePath.TrimStart('/')).Trim('/');

src/Elastic.Markdown/CrossLinks/IUriEnvironmentResolver.cs:24

  • [nitpick] Since GetBranch is now public and includes a new case for 'cloud', update its documentation/comments to clearly describe its behavior and branch mapping.
public static string GetBranch(Uri crossLinkUri)

tests/Elastic.Markdown.Tests/TestCrossLinkResolver.cs:49

  • Add test cases to verify the link resolution logic for the new 'cloud' repository scheme and to ensure both 'main' and 'master' branch scenarios are correctly handled.
DeclaredRepositories.AddRange(["docs-content", "kibana", "elasticsearch"]);

@Mpdreamz Mpdreamz merged commit 4344694 into main Mar 13, 2025
7 of 8 checks passed
@Mpdreamz Mpdreamz deleted the fix/hardcode-links-json-paths branch March 13, 2025 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants