Skip to content
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

lowercasing component names for es template #1323

Merged
merged 7 commits into from
Mar 31, 2021
Merged

lowercasing component names for es template #1323

merged 7 commits into from
Mar 31, 2021

Conversation

kgeller
Copy link
Contributor

@kgeller kgeller commented Mar 26, 2021

Correcting fieldset name capitalization when generating the generated es template

@kgeller kgeller linked an issue Mar 26, 2021 that may be closed by this pull request
@kgeller kgeller requested review from ebeahan and djptek March 26, 2021 19:18
typo in capitalization, fixed
djptek
djptek previously approved these changes Mar 29, 2021
Copy link
Contributor

@djptek djptek left a comment

Choose a reason for hiding this comment

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

Looks good

Copy link
Member

@ebeahan ebeahan left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this one, @kgeller!

A couple of items I wanted to note.

Testing

We don't have much testing coverage of the component and composable template portions of the Elasticsearch generator, but I think we should add test coverage for this case as we're modifying it a bit; at least some testing coverage for the uppercasing-to-lowercasing handling.

The existing tests are in scripts/tests/test_es_template.py for the ES template generator. The other test cases should be helpful examples, and I think since the casing change is pretty targeted, the nesting data structure can be pretty small. Something like:

        ecs_nested = {
            "Acme": {
                "name": "Acme",
            }
        }

Example shell script

As I was reviewing this change, I realized this example shell script for the composable/component index templates would also use the uppercase filename (e.g. Acme.json).

Maybe we can also add lowercasing of the file names in that example shellcode snippet with a command such as tr or awk?

Copy link
Member

@ebeahan ebeahan left a comment

Choose a reason for hiding this comment

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

Thanks! Added test looks good 👍

@kgeller kgeller merged commit b57eda5 into elastic:master Mar 31, 2021
@kgeller kgeller deleted the component-templates-lowercased branch March 31, 2021 15:06
kgeller added a commit that referenced this pull request Mar 31, 2021
lowercasing component names for es template
@ebeahan ebeahan added 1.10.0 and removed 1.x labels Apr 13, 2021
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.

Component templates names should be lowercased
3 participants