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

Add support for SIGABRT to KillProcessRequest_Signal, allow gNOI to generate coredumps #105

Merged
merged 1 commit into from
Feb 21, 2023

Conversation

aredmon8551
Copy link
Contributor

All currently defined signals only have an action of terminate, none generate a core. Proposing addition of SIGABRT to support core generation:
https://man7.org/linux/man-pages/man7/signal.7.html

system/system.proto Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Oct 31, 2022

Pull Request Test Coverage Report for Build 4236638936

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%

Totals Coverage Status
Change from base Build 4236524998: 0.0%
Covered Lines: 0
Relevant Lines: 0

💛 - Coveralls

@hellt
Copy link
Contributor

hellt commented Oct 31, 2022

@marcushines @aredmon8551 We also need bump the service version (and regen the protos?)

@marcushines
Copy link
Contributor

Please fix lint as well as generate protos

I don't know that you have to bump as this is not breaking

@hellt
Copy link
Contributor

hellt commented Nov 1, 2022

@marcushines shouldn't new functionality (like supporting a new signal) require a bump in the minor version of a semver-styled version string?

I.e. in our product documentation we aim to provide a service version which we support, i.e. 1.2.0 plus the commit hash which we used to get the protos from.
Now if a new signal is added while the version stays the same 1.2.0, someone may expect to use that signal, judging by the version match.

I think the version should be bumped 1.0.0 -> 1.1.0 to reflect the newly added backwards compatible functionality

Copy link
Contributor

@hellt hellt left a comment

Choose a reason for hiding this comment

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

@marcushines here is a suggestion set for fixing gofmt complaints. Although the question remains what protoc-gen code made it non-compliant? Looks weird

system/system.pb.go Outdated Show resolved Hide resolved
system/system.pb.go Outdated Show resolved Hide resolved
@aredmon8551
Copy link
Contributor Author

aredmon8551 commented Nov 29, 2022

I had to manually pull down the referenced projects in the compile.sh script to my $GOPATH because go get fails (as I believe no main.go file exists in those projects). Script ran without issues, though unsure if go install and go get should be used here.

marcushines added a commit to aredmon8551/gnoi that referenced this pull request Feb 21, 2023
Updated based on new proto generated files
Updated based on new proto generated files
@marcushines marcushines merged commit 1727ed9 into openconfig:main Feb 21, 2023
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