kie-issues#1352: Enforce reproducible build on kie-tools #2455
Conversation
There was a problem hiding this comment.
Hi @pefernan! Amazing solution. Glad to see you digging deep. I left some comments inline, especially trying to make it even more concise. Love the README.md notes and please don't forget to update them after incorporating changes.
| <property> | ||
| <name>reproducible</name> | ||
| </property> |
There was a problem hiding this comment.
Can we change the activation to
<activation>
<activeByDefault>false</activeByDefault>
</activation>
and use -P instead of -D to activate it on packages? I'm being extra careful here because managing profiles on a env-var-parameterized repo can become messy really quickly. I respect the extra care you had to only add it on build:prod scripts, but I'd love if we could do this extra step too.
There was a problem hiding this comment.
@tiagobento about -P and -D: I personally have no strong opinion (TBH, I also prefer the -P approach), but, as we agreed upon about other topic, we must consider all our codebase as a single unit, and strive for consistency. For some reason, the -Dapproach is used elsewhere, so I think it is better to enforce this kind of consistency. Needless to say that consistency and clear conventions is the key to avoid a manually crafted, long-time consuming maintenance (in that specific case, I think we must get to the point that all such cloned/copied stuff lives in one single place, and note duplicated over and over)
There was a problem hiding this comment.
|
|
||
| <profiles> | ||
| <profile> | ||
| <id>reproducible-build</id> |
There was a problem hiding this comment.
| <id>reproducible-build</id> | |
| <id>reproducible</id> |
?
There was a problem hiding this comment.
See my other comment on this file...
There was a problem hiding this comment.
@tiagobento this is exactly the same config used all over other repositories, included the reproducible-build id. Again, it is not important one way or the other, but it is important tha we have the same configurations all over our codebase: I already stumbled on the fact that the full flag has a slightly different meaning between some repository, and I want to avoid to increase such kind of entropy. Moreover, if, ever, we'll get to an easy and simple CI configuration, having the same profile named same way all over our code would be surely beneficial.
|
btw @gitgabrio this being in the |
tiagobento
added
lang:java
pefernan
force-pushed
the
kie-issues#1352
branch
from
f565c72
to
76a6317
Compare
|
@tiagobento in this case, just to keep the consistency with other repos, I'll +1 to @gitgabrio proposal to keep the profile as it is. But if there's a problem in CI I won't fight to change it |
There was a problem hiding this comment.
Hi @pefernan Thanks for all your effort in this PR.
I agree with @tiagobento to add the activeByDefault to false, even if it should be already implicitly so
| <property> | ||
| <name>reproducible</name> | ||
| </property> |
There was a problem hiding this comment.
…changes (#57) * Apply updates from apache#2635 * Updates from apache#2455 * Updates from apache#2376 * Fixed wrong value for SONATAFLOW_MANAGEMENT_CONSOLE_WEBAPP__sonataflowEnvMode
Closes apache/incubator-kie-issues#1352