Initialize buildpack #1
Conversation
|
Converting to draft b/c of 79b9e38#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R12-R14. Needs paketo-buildpacks/libpak#22 to merge |
nebhale
added
semver:major
| @@ -0,0 +1,71 @@ | |||
| /* | |||
There was a problem hiding this comment.
Generally since the helpers all get compiled into the same binary (helper), separate from the buildpack's binary (main), I've taken to isolating them all in a separate package. More a style to help navigate than anything technical.
There was a problem hiding this comment.
I noticed this pattern and ignored it b/c I wanted to unexported functions like 79b9e38#diff-313fab271285a81f27a170c3d599f1e86c757acd469d3479e765d0d1d1160174R31
This suggestion could be implemented with a little reorg (pulling out paths in main). But, given that the helper and the build for this buildpack are doing almost identical things (and require the same imports) it made sense to me that they would live in the same package.
There was a problem hiding this comment.
Fair enough. I'm concerned though about the unit testing implications of this being an un-exported function. And once you take that into account, keeping them together often doesn't matter.
| } | ||
| e.Logger.Infof("Added %d additional CA certificate(s) to system truststore", len(paths)) | ||
|
|
||
| if v := e.GetEnv(EnvCAPath); v == "" { |
There was a problem hiding this comment.
os.LookupEnv() please. I really hate == "" as an idiom around environment variables (you'll notice that I prefer [[ -z ${FOO+x} ]] pretty much everywhere).
There was a problem hiding this comment.
Wouldn't we still want to set it if it was set to an empty string though? We would end up with something like:
if v, ok := e.LookupEnv(EnvCAPath); !ok || v == "" {
There was a problem hiding this comment.
I wouldn't do the test against empty. If a user sets the value, but sets it to nothing, they deserve what they get.
cacerts/trusted_cas.go
Outdated
| layer.BuildEnvironment = libcnb.Environment{} | ||
|
|
||
| certsDir := filepath.Join(layer.Path, "certs") | ||
| if err := os.Mkdir(certsDir, 0777); err != nil { |
There was a problem hiding this comment.
We've used 0755 everywhere else in every other buildpack.
There was a problem hiding this comment.
Until the spec declares the required umask of the build container (I'll bet you'll tell me it does :/), we set exactly what we want. Consistency and predictability are key.
* Respects CA from bindings at build time * Contributes exec.d helper that does the same at runtime Signed-off-by: Emily Casey <[email protected]>
* formats code * add tests for GenerateHashLinks and CanonicalName * fail if cert file has muliple PEMs Signed-off-by: Emily Casey <[email protected]>
Signed-off-by: Emily Casey <[email protected]>
Signed-off-by: Emily Casey <[email protected]>
* Explicitly adds ca-cert-helper to build plan * Contructs a single CA certificates layer even if there are multiple ca-certficates plan entries * Readability improvements * License files **NOTE** uses replace directive for libpak, remove before merging Signed-off-by: Emily Casey <[email protected]>
* Adds missing newlines * Adds missing licenses
ekcasey
force-pushed
the
init
branch
3 times, most recently
from
a0880af to
509e0bd
Compare
* Refactors Detect * More consistent name and constants Signed-off-by: Emily Casey <[email protected]>
Signed-off-by: Emily Casey [email protected]