Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Align input options for --include and --subset arguments #1519

Merged
merged 21 commits into from
Dec 13, 2021
Merged

Align input options for --include and --subset arguments #1519

merged 21 commits into from
Dec 13, 2021

Conversation

djptek
Copy link
Contributor

@djptek djptek commented Jul 15, 2021

  • Have you signed the contributor license agreement? Y
  • Have you followed the contributor guidelines? Y
  • For proposing substantial changes or additions to the schema, have you reviewed the RFC process? Y
  • If submitting code/script changes, have you verified all tests pass locally using make test? Y
  • If submitting schema/fields updates, have you generated new artifacts by running make and committed those changes? Y
  • Is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed. Y
  • Have you added an entry to the CHANGELOG.next.md? Y

Addresses #899

@djptek djptek marked this pull request as draft July 15, 2021 12:27
@djptek
Copy link
Contributor Author

djptek commented Jul 15, 2021

In Draft as need to add tests

@djptek
Copy link
Contributor Author

djptek commented Jul 16, 2021

low level tests passing, working with include, exclude, subset. next step add higher level tests

@djptek djptek marked this pull request as ready for review October 20, 2021 12:54
@djptek djptek requested review from ebeahan and kgeller October 20, 2021 12:54
@@ -10,6 +10,8 @@ Thanks, you're awesome :-) -->

### Schema Changes

* Align input options for --include and --subset arguments #1519
Copy link
Contributor

Choose a reason for hiding this comment

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

This is more of a tooling addition than a schema change, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, thanks for spotting @kgeller

Copy link
Contributor

@kgeller kgeller left a comment

Choose a reason for hiding this comment

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

Looks great Dominic!

@djptek djptek marked this pull request as draft October 25, 2021 15:42
@djptek
Copy link
Contributor Author

djptek commented Oct 25, 2021

moving back to draft while I create additional tests for --subset

@djptek
Copy link
Contributor Author

djptek commented Oct 25, 2021

Checked Unit tests and I think coverage is OK, updated documentation to reflect changes from point of view of --subset

@djptek djptek marked this pull request as ready for review October 25, 2021 18:34
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.

One small observation, but otherwise LGTM!

Thanks for addressing, @djptek! It'll be nice to have this consistency.

djptek and others added 2 commits October 29, 2021 09:46
Co-authored-by: Eric Beahan <ebeahan@gmail.com>
@djptek
Copy link
Contributor Author

djptek commented Oct 29, 2021

Thanks for review @ebeahan, holding this until new issue #1646 is resolved

@ebeahan
Copy link
Member

ebeahan commented Dec 2, 2021

@djptek, with this change aligning the input behavior of these two arguments, can it be implemented independently from the @timestamp conversation happening in #1646?

@djptek
Copy link
Contributor Author

djptek commented Dec 9, 2021

@ebeahan I think I need to fix the @timestamp first in order otherwise this PR will permit non-conformant ECS with no @timestamp. I checked the tests and they are valid, it's just a question of order of execution

@ebeahan
Copy link
Member

ebeahan commented Dec 9, 2021

I think I need to fix the @timestamp first in order otherwise this PR will permit non-conformant ECS with no @timestamp

I believe that's already true in the current tooling and not something introduced by these changes?

If this change is ready, we can introduce your improvements here and address #1646 later.

@djptek djptek merged commit e74ff05 into elastic:main Dec 13, 2021
@ebeahan
Copy link
Member

ebeahan commented Dec 14, 2021

@djptek should this one be backported to 8.0?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants