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

[CWS] Rename activity profile field from is_exec_child to is_exec_exec #265

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lebauce
Copy link
Contributor

@lebauce lebauce commented Aug 1, 2023

What does this PR do?

Motivation

Additional Notes

Possible Drawbacks / Trade-offs

Describe how to test/QA your changes

Reviewer's Checklist

Reviewers: please see the review guidelines.

@lebauce lebauce requested review from a team as code owners August 1, 2023 16:46
@lebauce lebauce changed the title Rename activity profile field from is_exec_child to is_exec_exec [CWS] Rename activity profile field from is_exec_child to is_exec_exec Aug 1, 2023
@modernplumbing
Copy link

Is this bool still true if it's >2 execs?

@@ -88,7 +88,7 @@ message ProcessInfo {
repeated string envs = 19;
bool envs_truncated = 20;

bool is_exec_child = 21;
bool is_exec_exec = 21;
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Renaming fields is usually discouraged - why not create a new field and deprecate the old one? If we keep the name, will this break any expected serialized marshalling/unmarshalling based from this protobuf?

Sidenote: I'm not fully sure what is_exec_exec would mean in the context of a process so I'm hesitant to say that the new name is an improvement over the old one.

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.

4 participants