Skip to content

Add support for secure commands audit #80

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Feb 21, 2019
Merged

Add support for secure commands audit #80

merged 1 commit into from
Feb 21, 2019

Conversation

meskio
Copy link
Contributor

@meskio meskio commented Feb 21, 2019

And clean up the rest of the examples.

@davideschiera
Copy link
Contributor

I merged your branch to a temporary branch on the repository (https://github.com/draios/python-sdc-client/tree/commands_audit_tmp) and the build succeeded. I'll need to understand why the build fails on forks but succeeds on branches...

I'd ask you a favor though: Would you mind moving the changes to examples to a separate PR? It'd make it easier to review and keep the history log cleaner with more isolated/independent commits. Hope it makes sense!

Thanks for the help!

@meskio
Copy link
Contributor Author

meskio commented Feb 21, 2019

I merged your branch to a temporary branch on the repository (https://github.com/draios/python-sdc-client/tree/commands_audit_tmp) and the build succeeded. I'll need to understand why the build fails on forks but succeeds on branches..

Maybe the tokens are only available for the people that have rights in the repo.

I'd ask you a favor though: Would you mind moving the changes to examples to a separate PR? It'd make it easier to review and keep the history log cleaner with more isolated/independent commits. Hope it makes sense!

I just separated the pull-reqs, the one for the examples is: #81

Out of curiosity, why do you squash all my commits before merging them into master?

@davideschiera
Copy link
Contributor

Re travis: That might be the reason. Do you know how the Travis configuration could be changed to make tokens available to forks as well?

Re squash commits: Good question, and sorry if that caught you by surprise or is causing any issue. There are a couple of main reasons.
First, I personally like the clean master history if you squash commits (together with how Github prepares the commit message), it makes it easier to deal with when you need it in the future.
Second, more often than not, PRs will see temporary commits (do something, fix it, fix it again, etc. -- or at least this is very much my case!) which would mostly be noise in the master history, while a single commit will capture the changes as independent piece of work. Since you have the reference to the PR in the commit message, all the commits and discussion about that are one click away and well isolated from the rest of the work.

Hope this makes sense. I'm definitely open to change approach if you see advantages and interested to pick your brain about it!

Copy link
Contributor

@davideschiera davideschiera left a comment

Choose a reason for hiding this comment

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

Nice work!

@meskio
Copy link
Contributor Author

meskio commented Feb 21, 2019

Re travis: That might be the reason. Do you know how the Travis configuration could be changed to make tokens available to forks as well?

It might not be possible. If a fork could access the secret variable it could send it back and 'steal' it. You will need to make it public, but I'm not sure if you want to give everybody access to the testing sysdig API.

Encrypted environment variables are not available to pull requests from forks due to the security risk of exposing such information to unknown code.
https://docs.travis-ci.com/user/environment-variables/#defining-encrypted-variables-in-travisyml

Hope this makes sense. I'm definitely open to change approach if you see advantages and interested to pick your brain about it!

I'm fine with that approach. Every project have their own ways. I'll just try to open isolated pull-reqs instead of piling up commits in one.

@davideschiera davideschiera merged commit 63f47ed into sysdiglabs:master Feb 21, 2019
Copy link
Contributor

@figarocorso figarocorso left a comment

Choose a reason for hiding this comment

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

Better late than never... 👍

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

Successfully merging this pull request may close these issues.

3 participants