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

Pass "big" binary files #162 #174

Merged
merged 8 commits into from
Nov 8, 2017
Merged

Pass "big" binary files #162 #174

merged 8 commits into from
Nov 8, 2017

Conversation

col-panic
Copy link
Contributor

@col-panic col-panic commented Sep 15, 2017

I would appreciate feedback on the changes made. The issue itself is documented in #162 - a usage guide is provided in https://github.com/col-panic/webhook/wiki

@adnanh
Copy link
Owner

adnanh commented Nov 4, 2017

This looks great!

I would just change one little thing - instead of keeping file on exit by default, I would delete it by default.

So we can rename that option to "KeepFileOnExit".

@col-panic
Copy link
Contributor Author

While we can be sure that the file will be here on starting our hook, we can't be sure about the file still existing after the hook was run ( the script could remove the file by itself). So naming the option KeepFileOnExit would suggest that even though the file was already deleted, we will still be able to keep it. I think that this is misleading - due to this I named it DeleteOnExit ( while a more descriptive option name would be DeleteOnExitIfExists).

If you insist on your naming I can certainly go for it, but I found it more intuitive this way.

@adnanh
Copy link
Owner

adnanh commented Nov 6, 2017

Ah, you are absolutely correct.

I would then remove the flag and do this cleanup by default, after the hook command is done executing, if the file is there, delete it.

Since it's a tempfile, if the user wants to keep it, they should copy it to the appropriate place in their script code, because if we don't delete it, it's gonna be deleted after machine restart anyway.

@col-panic
Copy link
Contributor Author

Allright, I patched the code accordingly. I also updated the test file, yet I can't figure out why I still make messages like

./webhook_test.go:44: not enough arguments in call to handleHook
	have (*"github.com/adnanh/webhook/hook".Hook, *map[string]interface {}, *map[string]interface {}, *map[string]interface {}, *[]byte)
	want (*"_/Users/marco/Development/goclipse-latest/git/webhook/hook".Hook, string, *map[string]interface {}, *map[string]interface {}, *map[string]interface {}, *[]byte)
./webhook_test.go:74: not enough arguments in call to handleHook
	have (*"github.com/adnanh/webhook/hook".Hook, *map[string]interface {}, *map[string]interface {}, *map[string]interface {}, *[]byte)
	want (*"_/Users/marco/Development/goclipse-latest/git/webhook/hook".Hook, string, *map[string]interface {}, *map[string]interface {}, *map[string]interface {}, *[]byte)
./webhook_test.go:98: not enough arguments in call to handleHook
	have (*"github.com/adnanh/webhook/hook".Hook, *map[string]interface {}, *map[string]interface {}, *map[string]interface {}, *[]byte)
	want (*"_/Users/marco/Development/goclipse-latest/git/webhook/hook".Hook, string, *map[string]interface {}, *map[string]interface {}, *map[string]interface {}, *[]byte)
FAIL	_/Users/marco/Development/goclipse-latest/git/webhook [build failed]
--- FAIL: TestCheckPayloadSignature256 (0.00s)
	hook_test.go:53: failed to check payload signature {"{\"a\": \"z\"}", "secret", "sha256f417af3a21bd70379b5796d5f013915e7029f62c580fb0f500f59a35a6f04c89"}:
		expected {mac:"f417af3a21bd70379b5796d5f013915e7029f62c580fb0f500f59a35a6f04c89", ok:true},
		got {mac:"f417af3a21bd70379b5796d5f013915e7029f62c580fb0f500f59a35a6f04c89", ok:false}
	hook_test.go:57: error message should not disclose expected mac: invalid payload signature sha256f417af3a21bd70379b5796d5f013915e7029f62c580fb0f500f59a35a6f04c89
FAIL

on make test

@adnanh
Copy link
Owner

adnanh commented Nov 7, 2017

Ahh, the #173 forgot to include request ID in the webhook_test.go file, I've fixed it now, pull the latest master into your branch :-)

@col-panic
Copy link
Contributor Author

done!

@adnanh
Copy link
Owner

adnanh commented Nov 8, 2017

Can you just choose adnanh:development as base branch so I can merge it in :-)

@adnanh
Copy link
Owner

adnanh commented Nov 8, 2017

And update the wiki with the new option

@col-panic col-panic changed the base branch from master to development November 8, 2017 09:11
@col-panic
Copy link
Contributor Author

Ok, I changed the base, after you accepted the PR I plan to integrate the documentation from my page into your wiki in an updated version!

@adnanh adnanh merged commit ba0adb1 into adnanh:development Nov 8, 2017
@adnanh
Copy link
Owner

adnanh commented Nov 8, 2017

Done!

@adnanh adnanh mentioned this pull request Nov 8, 2017
@col-panic
Copy link
Contributor Author

Docu added :)

@adnanh
Copy link
Owner

adnanh commented Nov 8, 2017

Awesome, thank you very much for your contribution and making webhook a better tool!

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.

2 participants