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

Extend file.pe Fieldset #1071

Merged
merged 57 commits into from
Feb 8, 2021
Merged

Extend file.pe Fieldset #1071

merged 57 commits into from
Feb 8, 2021

Conversation

peasead
Copy link
Contributor

@peasead peasead commented Nov 3, 2020

Issue

Resolves #1039

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

RFC Preview

@peasead peasead self-assigned this Nov 3, 2020
@webmat webmat added RFC and removed RFC:candidate labels Nov 5, 2020
CHANGELOG.next.md Outdated Show resolved Hide resolved
rfcs/text/pe/pe.yml Outdated Show resolved Hide resolved
rfcs/text/pe/pe.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

This is looking really good.

For stage 1 we don't need to have all fields hashed out. So let's discuss them a bit, but if any of the discussions on the field end up being thorny, we can capture these concerns in the RFC and this shouldn't be a blocker for stage 1.

rfcs/text/0000-extend-file-pe.md Outdated Show resolved Hide resolved
rfcs/text/0000-extend-file-pe.md Outdated Show resolved Hide resolved
rfcs/text/0000-extend-file-pe.md Outdated Show resolved Hide resolved
rfcs/text/0000-extend-file-pe.md Outdated Show resolved Hide resolved
rfcs/text/0000-extend-file-pe.md Outdated Show resolved Hide resolved
rfcs/text/0000-extend-file-pe.md Outdated Show resolved Hide resolved
rfcs/text/0000-extend-file-pe.md Outdated Show resolved Hide resolved
rfcs/text/0000-extend-file-pe.md Outdated Show resolved Hide resolved
rfcs/text/0000-extend-file-pe.md Outdated Show resolved Hide resolved
rfcs/text/0000-extend-file-pe.md Outdated Show resolved Hide resolved
rfcs/text/pe/pe.yml Outdated Show resolved Hide resolved
peasead and others added 4 commits November 6, 2020 10:37
Co-authored-by: Mathieu Martin <webmat@gmail.com>
Co-authored-by: Mathieu Martin <webmat@gmail.com>
Co-authored-by: Mathieu Martin <webmat@gmail.com>
Co-authored-by: Mathieu Martin <webmat@gmail.com>
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.

I took another pass. The main item I see remaining before advancing is capturing the outstanding questions/concerns within the RFC doc for future discussion.

@andrewstucki @rw-access any other feedback here?

rfcs/text/0000-extend-file-pe.md Outdated Show resolved Hide resolved
rfcs/text/pe/pe.yml Outdated Show resolved Hide resolved
rfcs/text/pe/pe.yml Outdated Show resolved Hide resolved
rfcs/text/pe/pe.yml Outdated Show resolved Hide resolved
rfcs/text/pe/pe.yml Outdated Show resolved Hide resolved
peasead and others added 3 commits January 13, 2021 10:50
Co-authored-by: Eric Beahan <ebeahan@gmail.com>
updated types and packers to `normalize: array`
removed hashing algorithms that exist under `hast.*`.
@andrewstucki
Copy link
Contributor

@ebeahan / @peasead -- sorry about the delay, I'll take another pass at the pe/macho/elf fields and see if anything jumps out to me

Copy link
Contributor

@andrewstucki andrewstucki left a comment

Choose a reason for hiding this comment

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

Generally this looks pretty good to me as far as the initial fields to fight over are concerned 😅

I'll leave it to @ebeahan to opine on whatever additional formatting for the RFC doc itself might need to be done prior to merge.

rfcs/text/0000-extend-file-pe.md Outdated Show resolved Hide resolved
@peasead
Copy link
Contributor Author

peasead commented Feb 5, 2021

@ebeahan just checking on this to see what the next steps are.

@ebeahan
Copy link
Member

ebeahan commented Feb 5, 2021

I made some minor housekeeping edits to the field definitions:

  • pe.exports - Since the description indicates this can be a list, I made the explicit.
  • pe.resources.type - Change example value to use array.
  • pe.machine_type - Switched to one example value since this field doesn't appear to require an array.
  • pe.packers - Also set example value to be an array.

Unless there are any issues with these changes, I think we're good for this proceeding with advancing 🎉

@peasead
Copy link
Contributor Author

peasead commented Feb 6, 2021

Thanks @ebeahan glad to be moving this along!

Thanks for those changes. 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.

Made the last set of changes prior to merging:

  • Assigned RFC number and advance date
  • Updated the stage name from proposal to draft to align with the new stages
  • Updated the markdown comments as well to align with the new stages (primarily removing the legacy stage 4 sections)

@ebeahan ebeahan merged commit 97cd176 into elastic:master Feb 8, 2021
@ebeahan
Copy link
Member

ebeahan commented Feb 8, 2021

@peasead Now that this is merged, I recommend opening the stage 2 PR even if only the stage number is updated. By having that PR open in advance, we have a spot to capture feedback or have more discussion.

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.

Extend file.pe
6 participants