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

[RFC] Stage 1 for volume device #2229

Merged
merged 13 commits into from
Jul 27, 2023
Merged

Conversation

Trinity2019
Copy link
Contributor

@Trinity2019 Trinity2019 commented Jun 29, 2023

  • 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? - N/A
  • If submitting schema/fields updates, have you generated new artifacts by running make and committed those changes? - N/A
  • Is your pull request against main? Unless there is a good reason otherwise, we prefer pull requests against main and will backport as needed. - Yes
  • Have you added an entry to the CHANGELOG.next.md? - N/A

@Trinity2019 Trinity2019 requested a review from a team as a code owner June 29, 2023 06:41
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.

In the markdown of the proposal doc, there are comments outlining the additional detail to capture for each stage. Can those sections be filled out for Stage 1?

@@ -106,6 +106,7 @@ The following are the people that consulted on the contents of this RFC.

* @Trinity2019 | author
* @ricardoelastic | reviewer
Copy link
Member

Choose a reason for hiding this comment

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

Should either @ricardoelastic or @stanek-michal be added as reviewers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I'll ask them in slack as well

@@ -29,13 +29,8 @@ This RFC propose adding the volume device fieldset to describe volume storage de
* volume.vendor_name
* volume.serial_number
* volume.volume_device_type
* volume.action
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed action because I plan to use event.action for logging the information

Copy link

@ricardoungureanu ricardoungureanu left a comment

Choose a reason for hiding this comment

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

LGTM

@Trinity2019
Copy link
Contributor Author

@ebeahan can we get this PR merged?

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.

Adding a few more observations as comments after another read through.

rfcs/text/0040-volume-device.md Outdated Show resolved Hide resolved
rfcs/text/0040-volume-device.md Outdated Show resolved Hide resolved
rfcs/text/0040-volume-device.md Outdated Show resolved Hide resolved
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.

LGTM

]
},
"message": "Endpoint volume device event",
"volume.bus_type": "FileBackedVirtual",
Copy link
Member

Choose a reason for hiding this comment

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

No need to fix for this round, but in a future stage let's correct the example to use an object vs. the dot notation for clarity.

Suggested change
"volume.bus_type": "FileBackedVirtual",
"volume": {
"bus_type": "FileBackedVirtual",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @ebeahan for the suggestions:) will fix in the next round.

@ebeahan ebeahan added the RFC label Jul 27, 2023
@ebeahan ebeahan merged commit 630429e into elastic:main Jul 27, 2023
1 check passed
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.

4 participants