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

Implement DeleteTracingPolicy functionality #1253

Merged

Conversation

inliquid
Copy link
Contributor

@inliquid inliquid commented Jul 19, 2023

  • Implement DeleteTracingPolicy for gRPC service
  • Implement "delete" command for tetra client
  • Rename DelTracingPolicy -> DeleteTracingPolicy everywhere for consistency

Fixes #1110

@inliquid inliquid requested review from mtardy and a team as code owners July 19, 2023 18:02
@netlify
Copy link

netlify bot commented Jul 19, 2023

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit ae042f6
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/64b977af150f600008c5c643
😎 Deploy Preview https://deploy-preview-1253--tetragon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@inliquid inliquid changed the title Pr/inliquid/remove-tracing-policy-method Implement DelTracingPolicy functionality Jul 19, 2023
Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

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

Hey 👋 thanks a lot for taking the time to write this PR, that's really nice of you!

If I may have a nit, would it be possible to keep delete instead of del everywhere (even in tetra)? I'm okay with having the whole delete word and changing this will unfortunately introduce a breaking change to the gRPC API (minimal I admit because it was not really exposed through a RPC but still).

@mtardy
Copy link
Member

mtardy commented Jul 19, 2023

Taking the need for the change to also warn you that you will need to add your "sign-off" to the commit. Just a formality:

It looks like your commit is not signed-off, to improve tracking of who did what, the Tetragon project uses a "sign-off" procedure which you can learn more about in the Cilium project documentation.

Concretely it consists of adding your real name and an email address to your git config and using git commit -s to write your commit message. You can use git commit --amend -s to add the signature to existing commits.

See more details on how to comply with the "sign-off" procedure

To add your name and email to your git config:

git config user.email "mail@example.com"
git config user.name "firstname lastname"

Note: add the --global flag after config to configure this by default for repositories on your host.

Then you can use for new commits:

git commit -s

Or for existing commits on which you want to add the signed-off-by statement:

git commit --amend -s

For more information, see this extract from git-commit(1) man page regarding the -s flag:

-s, --signoff, --no-signoff
   Add a Signed-off-by trailer by the committer at the end of the commit log
   message. The meaning of a signoff depends on the project to which you're
   committing. For example, it may certify that the committer has the rights to
   submit the work under the project's license or agrees to some contributor
   representation, such as a Developer Certificate of Origin. (See
   http://developercertificate.org for the one used by the Linux kernel and Git
   projects.) Consult the documentation or leadership of the project to which
   you're contributing to understand how the signoffs are used in that project.

   The --no-signoff option can be used to countermand an earlier --signoff
   option on the command line.

@mtardy
Copy link
Member

mtardy commented Jul 19, 2023

And lastly, you can squash your commit and prefix the message with something like gRPC: implement DeleteTracingPolicy functionality.

I hope all of this is not too overwhelming, please feel free to ask any questions if needed. Again grateful that you took the time to implement this! :)

@inliquid inliquid force-pushed the pr/inliquid/remove-tracing-policy-method branch from 7f09b1e to 20f7fe7 Compare July 19, 2023 20:44
@inliquid inliquid changed the title Implement DelTracingPolicy functionality Implement DeleteTracingPolicy functionality Jul 19, 2023
@inliquid
Copy link
Contributor Author

Hey @mtardy, thank you for detailed explanations, I think I've fixed these points, is there anything else I can do?

@mtardy
Copy link
Member

mtardy commented Jul 19, 2023

Hey @mtardy, thank you for detailed explanations, I think I've fixed these points, is there anything else I can do?

It's all good thanks a lot, let's run the CI and maybe ask for a review by Kornilios with who you interacted in the issue!

@mtardy mtardy requested a review from kkourt July 19, 2023 20:53
Copy link
Contributor

@kkourt kkourt left a comment

Choose a reason for hiding this comment

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

Thanks!

I have a comment, please see bellow.

cmd/tetra/tracingpolicy/tracingpolicy.go Outdated Show resolved Hide resolved
Signed-off-by: Alex In <reexistent@gmail.com>
@inliquid inliquid force-pushed the pr/inliquid/remove-tracing-policy-method branch from ae042f6 to f85adb0 Compare July 20, 2023 18:15
Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

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

Otherwise, for me, it's ready, any opinion @kkourt?

cmd/tetra/tracingpolicy/tracingpolicy.go Show resolved Hide resolved
Copy link
Contributor

@kkourt kkourt left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! 🚀

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.

Add DeleteTracingPolicy method to API
4 participants