Fix rendering of external groups in READMEs #3088
Conversation
|
test integrations |
|
Created or updated PR in integrations repository to test this version. Check elastic/integrations#16027 |
|
This change produces new issues, moving to draft by now. |
Pull request was converted to draft
Documentations rendering was looking for fields in the built directory, there the external fields have been already resolved and they don't include the external key. For legacy reasons, elastic-package only renders groups in READMEs when they are external, so these fields were not being rendered.
jsoriano
force-pushed
the
fix-rendering-external-groups
branch
from
01b0fa1 to
8bee428
Compare
|
test integrations |
|
Ok, last commit seemed to work, going on with the PR. |
|
Created or updated PR in integrations repository to test this version. Check elastic/integrations#16027 |
| } | ||
| } | ||
|
|
||
| func TesRenderReadmeWithFields(t *testing.T) { |
There was a problem hiding this comment.
These tests were not being executed because the function doesn't start with Test 🙁 After fixing the name, the tests weren't working.
| finder := packageRoot{from: fieldsParentDir} | ||
| return createValidatorForDirectoryAndPackageRoot(fieldsParentDir, finder, opts...) |
There was a problem hiding this comment.
These changes are needed for tests, but I guess it is fine to look for the package root from the fields directory instead of from the current directory?
There was a problem hiding this comment.
the finder is needed to validate the condition from https://github.com/elastic/elastic-package/pull/3088/files#diff-36d325d58b4ea8957abc63576d2e04d30ec8e58435ba06d442cb7923af063ad3L291 and throw an error if dependency management is enabled and there is no package root...
I suggest to inject the function as a param func(string) (string,error) and use the implementation packages.FindPackageRootFrom for the code and use mocks for the tests. we could inject a mock and assert if the finder is called or not
i also thought of getting the packageRoot and pass it through the function as a string param, but this operation is only executed when there is dependency management.
There was a problem hiding this comment.
the finder is needed to validate the condition from https://github.com/elastic/elastic-package/pull/3088/files#diff-36d325d58b4ea8957abc63576d2e04d30ec8e58435ba06d442cb7923af063ad3L291 and throw an error if dependency management is enabled and there is no package root...
Yes, this is working now. This is why I needed to update createManifestFile in tests to generate a more valid manifest.
I suggest to inject the function as a param
func(string) (string,error)and use the implementationpackages.FindPackageRootFromfor the code and use mocks for the tests. we could inject a mock and assert if the finder is called or not
Something like this is done in the tests for this function, passing the "finder" to createValidatorForDirectoryAndPackageRoot. I would not change the public CreateValidatorForDirectory unless this is introducing some issue, at least on this PR.
i also thought of getting the packageRoot and pass it through the function as a string param, but this operation is only executed when there is dependency management.
Dependency management is used basically always in real use cases 🙂
We can definitely apply more refactors around paths, but not sure if more refactors are needed here. I think we should review the location manager, place there all the logic we have to find paths and define interfaces that other components can use.
| docsFolder, err := createDocsFolder(packageRoot) | ||
| if err != nil { | ||
| return err | ||
| func TestUpdateReadmeWithFields(t *testing.T) { |
There was a problem hiding this comment.
This test reproduces the issue that this PR fixes.
internal/docs/readme.go
Outdated
| // the readme template reads data from the sourceFilesRoot directory. | ||
| // sourceFilesRoot is usually the package root when generating readme for checking up-to-dateness, | ||
| // and the built package root when generating readme for the built package. | ||
| func generateReadme(fileName, linksFilePath, packageRoot, sourceFilesRoot string) ([]byte, bool, error) { |
There was a problem hiding this comment.
| func generateReadme(fileName, linksFilePath, packageRoot, sourceFilesRoot string) ([]byte, bool, error) { | |
| func generateReadme(fileName, linksFilePath, sourceFilesRoot string) ([]byte, bool, error) { |
packageRoot and sourceFilesRoot is the same / should be the same value from the source of the package. I was confused when this function is called at updateReadme I was passing along the build source instead.
There was a problem hiding this comment.
You are right, packageRoot is not used now, removing it in 085b41e.
There was a problem hiding this comment.
overall, i think we should come up with a "convention" on how to call things:
- sourceFilesRoot == packageRoot
- the "buildFolderRoot"
i think i messed up trying to call things by their name.
Also, for the renderExportedFields(fieldsParentDir string) that is a data stream root path? but i see some examples where this is not a data stream path
"fields": func(args ...string) (string, error) {
if len(args) > 0 {
dataStreamPath := filepath.Join(sourceFilesRoot, "data_stream", args[0])
return renderExportedFields(dataStreamPath)
}
return renderExportedFields(sourceFilesRoot)
},
💚 Build Succeeded
History
cc @jsoriano |
4 participants
Swap source and build package roots when updating READMEs, so the generator looks for fields in the source directory. In the build directory the external fields have been already resolved and they don't include the
externalkey.For legacy reasons, elastic-package only renders groups in READMEs when they are external, so these fields were not being rendered.
See
elastic-package/internal/docs/exported_fields.go
Line 131 in d38ab6c
Naming of related variables is reviewed for consistency, so it is harder to misuse them.
Fix also related tests, and introduce new ones that reproduce the issue.