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

Simplify environment variable object for more compact data representation; removes truncation indicator #1288

Merged
merged 3 commits into from
Dec 17, 2024

Conversation

antchan2
Copy link
Contributor

@antchan2 antchan2 commented Dec 17, 2024

Related Issue:

Fixes ocsf-schema#1272

Description of changes:

Simplify data representation for environment variables array by changing the type of name and value from long_string to string. This removes truncation indicators from those fields but this is a worthwhile trade-off to vastly simplify the general case since truncation is a rare occurrence. environment_variable remains a separate class (i.e., not reuse the Key:Value object) so it can be extended to more elegantly handle truncation, if there turns out to be a real need, in the future.

Screenshot 2024-12-13 at 10 34 06 AM Screenshot 2024-12-13 at 10 33 52 AM

…tion; removes truncation indicator (#9)

Signed-off-by: Anthony Chan <antchan2@cisco.com>
Signed-off-by: Anthony Chan <antchan2@cisco.com>
@antchan2 antchan2 force-pushed the simplify_env_staging branch from c0eac64 to 3cf9d3d Compare December 17, 2024 17:21
@mlmitch
Copy link
Contributor

mlmitch commented Dec 17, 2024

I like it

Copy link
Contributor

@zschmerber zschmerber left a comment

Choose a reason for hiding this comment

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

LGTM

@floydtree
Copy link
Contributor

Do we wanna remove the long_string object as well? We can add it down the road if needed. Or do folks see value in it? @mikeradka @zschmerber @davemcatcisco

@mikeradka mikeradka self-requested a review December 17, 2024 18:07
@zschmerber zschmerber merged commit 9d45de8 into ocsf:main Dec 17, 2024
3 checks passed
@mikeradka
Copy link
Contributor

Do we wanna remove the long_string object as well? We can add it down the road if needed. Or do folks see value in it? @mikeradka @zschmerber @davemcatcisco

Hm, good point. I am sure there could be potential down the road for this object, but as of this PR this object would be unreferenced. I suppose since this is merged now, we could either do a subsequent PR to remove this object, or keep the unreferenced object. Maybe in the spirit of keeping the schema clean, we do a follow up PR to remove the long_string objects. Thoughts @davemcatcisco @floydtree @zschmerber ?

@antchan2
Copy link
Contributor Author

long_string is still used for script content

@mikeradka
Copy link
Contributor

long_string is still used for script content

Ah, good eye. I think that answers that!

@floydtree
Copy link
Contributor

@antchan2 Ah, good catch! Thanks.

@davemcatcisco
Copy link
Contributor

Yes, script content was the original reason for adding it, I think.

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

Successfully merging this pull request may close these issues.

Process' environment variables array can be simplified
6 participants