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

[QA][Code Coverage] Change location of Team Assignment Definition #72150

Closed
wants to merge 2 commits into from

Conversation

wayneseymour
Copy link
Member

@wayneseymour wayneseymour commented Jul 16, 2020

Move the json def to easier location, such that devs can easily find it, change it and include those
changes in their PR's.

such that devs can easily find it, change it and include those
changes in their PR's.
@wayneseymour wayneseymour added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes test-coverage issues & PRs for improving code test coverage backport:skip This commit does not require backporting labels Jul 16, 2020
@wayneseymour wayneseymour self-assigned this Jul 16, 2020
@wayneseymour
Copy link
Member Author

@wayneseymour wayneseymour marked this pull request as ready for review July 16, 2020 22:57
@wayneseymour wayneseymour requested a review from a team as a code owner July 16, 2020 22:57
@LeeDr
Copy link

LeeDr commented Jul 17, 2020

I'm thinking we should also rename it while we're at it. ingestion_pipeline_painless.json doesn't really make it clear what it is. Could we rename it to something like code_coverage_team_assignment.json ?

@wayneseymour
Copy link
Member Author

Yes sir!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link

@LeeDr LeeDr left a comment

Choose a reason for hiding this comment

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

LGTM - Please check with operations team to make sure they don't have a problem with files at the root level. But I like it in the easiest to find place as possible.

@brianseeders brianseeders requested a review from a team July 17, 2020 20:01
@spalger
Copy link
Contributor

spalger commented Jul 17, 2020

such that devs can easily find it, change it and include those changes in their PR's

I don't think the location of this file is the right issue to fix. I really don't want another file at the root of the repo and this file specifically feels like it should be auto-generated from something like the codeowners file and be fixed to not be a massive json encoded string that nobody could possibly read or modify correctly without careful find-replace or knowing how to properly "de-compress" and "re-compress".

@wayneseymour
Copy link
Member Author

such that devs can easily find it, change it and include those changes in their PR's

I don't think the location of this file is the right issue to fix. I really don't want another file at the root of the repo and this file specifically feels like it should be auto-generated from something like the codeowners file and be fixed to not be a massive json encoded string that nobody could possibly read or modify correctly without careful find-replace or knowing how to properly "de-compress" and "re-compress".

Yeah I concur. The process is to use the Kibana Dev Tools to edit it, then use the auto indent to one-line it. Maybe we should expose the code that does the formatting. Kinda looks like its tied to the Ace Editor though

@wayneseymour
Copy link
Member Author

@spalger yeah I totally agree about the auto-gen from codeowners. I actually started coding that like 3 - 4 months ago but I scrapped it. We'd still have to expose the formatting code, imho.

@spalger
Copy link
Contributor

spalger commented Jul 17, 2020

The way we handle this for things like the renovate config is there is a module which generates the renovate config, commit the output of that script to the repo, and then validate that the generated output is up-to-date in CI. When the output does end up being out of date CI "educates" people about what they should do. Do you think we could implement something similar to that for this config file? I don't think adding it to the root of the repo is going to accomplish anything other than creating more clutter personally.

@wayneseymour
Copy link
Member Author

@spalger oh totally down to not have it in the root.

Maybe we should go ahead and gen from code owners, but not sure how we'd validate or even update, via http. The reason I put in the src code a one-liner is that it fails to HTTP PUT when it's in the half-json format

@spalger
Copy link
Contributor

spalger commented Jul 17, 2020

Well, the idea would be that we'd use codeowners, or some other readable, maintainable, config format and then generate the painless script from that config. Then we would still have the painless script that's compatible with the ES API but would have a maintainable file that contributors can help maintain. If we can't use codeowners as the "readable, maintainable, config format" then maybe whatever we use instead could also generate the codeowners file...

CI would be used to make sure the config file and the generated painless script are in sync.

@wayneseymour
Copy link
Member Author

ahhh, I think I'm starting to track what ya mean. You know it's Friday so my brain is lazy.
I do like that.

@wayneseymour
Copy link
Member Author

@spalger I'll follow up with my boss write an issue on this. That way, we can track the what, why and how :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes test-coverage issues & PRs for improving code test coverage v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants