-
-
Notifications
You must be signed in to change notification settings - Fork 286
don't put signatures in deploy jar #229
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
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
Can one of the admins verify this patch? |
test this please (cibot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have a test for this change so it won't be broken by mistake in the future. We can even check in a small binary with a signature if that would make it easier. WDYT?
@Override | ||
protected boolean ignoreFileName(String name) { | ||
/* | ||
* It does not make sense to copy signature files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it would be wise to rename JarCreator
to FatJarCreator
/ DeployJarCreator
/ UberJarCreator
? Current name made me wonder why the override is like this and the base is just "false". Until I read the comment and remembered
yeah, I will add a test. I just wanted to get unblocked so I pushed it here. |
Sure. We can merge and you can add a test later if you're blocked. WDYT
about the JarCreator class name (again not blocking the merge)?
…On Wed, Jun 14, 2017 at 9:58 PM P. Oscar Boykin ***@***.***> wrote:
yeah, I will add a test. I just wanted to get unblocked so I pushed it
here.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#229 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIF1YzhB97IN-fC8gsYgFa0tBoFaJ8ks5sEC1RgaJpZM4N6R19>
.
|
okay @ittaiz, I have added a test which exposes the problem if I don't have this change and does not fail with the change. I don't want to rename things at this stage since this code was originally copied from bazel and they set the names (also, we have code that actually depend on it elsewhere so it will be a migration pain for us that I am hoping to avoid). |
I consent |
Test this please |
LGTM |
* don't put signatures in fat jar * Add tests
We hit some error with a jar including a signature, then it causing the deploy jar to fail verification.