-
Notifications
You must be signed in to change notification settings - Fork 28
Product Configuration #1865
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?
Product Configuration #1865
Conversation
…roduct metadata, and substitution support for them
# Conflicts: # config/legacy-url-mappings.yml # src/tooling/docs-assembler/Cli/RepositoryCommands.cs
config/legacy-url-mappings.yml
Outdated
@@ -1,82 +1,228 @@ | |||
############################################# | |||
# This file defines the legacy URL mappings for the documentation site. | |||
# It maps current documentation pages to older versions to ensure users can find the content they need. | |||
# TODO: Refactor the model, for now we just use the first element as current version | |||
# Upon creation of version selection dropdowns, this data is combined with the current version of its product. | |||
############################################# | |||
|
|||
stack: &stack [ '9.0+', '8.19', '8.18', '8.17', '8.16', '8.15', '8.14', '8.13', '8.12', '8.11', '8.10', '8.9', '8.8', '8.7', '8.6', '8.5', '8.4', '8.3', '8.2', '8.1', '8.0', '7.17' ] |
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.
Why is 9.0+
still here?
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.
Question also applies to other legacy_versions
.
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.
Oh, that didn't go.
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.
What about other legacy versions? E.g. apm agent rum also contains the current version. Others might too.
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.
@elastic/docs-engineering I think this PR would also solve #1497
What happens with existing product subs define in docset.yml files, by the way? Should we prefer products.yml driven substitutions from now on?
docs/configure/site/products.md
Outdated
@@ -0,0 +1,37 @@ | |||
# `products.yml` | |||
|
|||
The [`products.yml`](https://github.com/elastic/docs-builder/blob/main/config/products.yml) file specifies metadata regarding the projects in the organization leveraging v3. |
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.
The [`products.yml`](https://github.com/elastic/docs-builder/blob/main/config/products.yml) file specifies metadata regarding the projects in the organization leveraging v3. | |
The [`products.yml`](https://github.com/elastic/docs-builder/blob/main/config/products.yml) file specifies metadata regarding the projects in the organization that use the V3 docs system. |
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.
"leveraging v3" doesn't say much. Could you apply the suggested edit? 🙏🏻
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.
A few small nits and q's.
Would be good to include more tests on resolving product names and their attached versioning schemes.
docs/configure/site/products.md
Outdated
|
||
| Substitution | Result | | ||
| --- |---| | ||
| `{{ product.apm-agent-dotnet }}` |{{ product.apm-agent-dotnet }} | |
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.
In a follow up it would be cool to be able to do a version
muations on these e,g
{{ .apm-agent-ios | version }}
and {{ .apm-agent-ios | version | M.M }}
etcetera.
@@ -105,6 +107,17 @@ This is dictated by the [`versions.yml`](https://github.com/elastic/docs-builder | |||
* `edot_python` | |||
* `edot_cf_aws` | |||
* `edot_collector` | |||
* `apm_attacher` | |||
* `apm_lambda` | |||
* `ecs_logging_dotnet` |
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.
Should we use kebab-case
for versioning schemes as well to match product keys?
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.
I went the opposite way just now - We already have docs in other repos (opentelemetry for example) using lowercase. I did kebab first to follow the repo names closely. Kinda on the fence for that one.
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.
yeah kebab-case
feels more natural.
Can the parsing be lenient to camel_case
but always emit kebab-case
?
@@ -42,7 +43,7 @@ public class AssembleSources | |||
|
|||
public FrozenDictionary<Uri, NavigationTocMapping> NavigationTocMappings { get; } | |||
|
|||
public FrozenDictionary<string, IReadOnlyCollection<string>> LegacyUrlMappings { get; } | |||
public LegacyUrlMappingConfiguration LegacyUrlMappings { get; } |
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.
Good cleanup 👍
public VersioningSystem? VersioningSystem { get; init; } | ||
} | ||
|
||
public sealed class ProductEqualityComparer : IEqualityComparer<Product>, IComparer<Product> |
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.
Missing tests for this, Add a comment why we need custom equality and comparison checks for Product
.
@@ -23,6 +26,10 @@ public partial class ConfigurationFileProvider | |||
private readonly IFileSystem _fileSystem; | |||
private readonly string _assemblyName; | |||
|
|||
internal IDeserializer Deserializer { get; } = new StaticDeserializerBuilder(new YamlStaticContext()) |
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.
Should this use the DocsBuilderStaticContext
as per https://github.com/elastic/docs-builder/pull/1865/files#diff-1cf450b2111680a6b66c82509cb42a43b384ae2a09c257b684aec2a28045e245R20
If so should we make this static and update the linked usage as well?
Co-authored-by: Fabrizio Ferri-Benedetti <[email protected]>
Fabrizio with the save. Co-authored-by: Fabrizio Ferri-Benedetti <[email protected]>
Smoke tests are failing. We should run a full assembler build to validate. |
…ture/product_config
…frontmatter definitions
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.
I think we're adding possible confusion points in the products.yml file with duplicates or nearly duplicate entries. LMK what you think.
self: | ||
display: 'Elastic Stack' | ||
versioning: 'stack' | ||
serverless: |
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.
What's the difference with cloud_serverless
? I think we only need one
serverless_elasticsearch: | ||
display: 'Elasticsearch' | ||
versioning: 'all' | ||
serverless_observability: | ||
display: 'Elastic Observability' | ||
versioning: 'all' | ||
serverless_security: | ||
display: 'Elastic Security' | ||
versioning: 'all' |
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.
Can you clarify how these behave? They're the solutions, on serverless. We have keys for the solutions already a bit above.
In terms of products, Elastic Observability, Elastic Security, and Elasticsearch can have 'stack' and 'serverless' versioning.
Unless we want to treat them as separate products, in which case the display name for the Serverless ones should be "Elastic Observability Serverless" "Elastic Security Serverless" "Elasticsearch Serverless".
self: | ||
display: 'Elastic Stack' | ||
versioning: 'stack' |
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.
self: | |
display: 'Elastic Stack' | |
versioning: 'stack' | |
self: | |
display: 'Self-managed Elastic' | |
versioning: 'stack' |
I think this is more accurate and makes a better distinction with the 'stack' key.
Handles #1637
This PR performs a few refactorings into how we deal with product, versioning and legacy URL mapping definitions.
Configuration
products.yml
andlegacy-url-mappings.yml
occurs during early service bootstrapping.Products Listing
products.yml
file to gather essential product metadata. Currently, we have a friendly label for each, and reference the versioning system adopted by the project in question.Versioning
Legacy Url Mapping
legacy-url-mappings.yml
has been reviewed to include a reference to a product. Most entries have a direct correlation. For a few cases, a best-match approach was taken.Docs