-
Notifications
You must be signed in to change notification settings - Fork 32
fix: hard fail on invalid railpack.json #226
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
base: main
Are you sure you want to change the base?
Conversation
37b4f94
to
98bfe70
Compare
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.
What "generated railpack.json" file are you talking about? I kind of think that we should still be attempting to build if the users config file is malformed. Though could go either way
This is hand-wavy on my end because I haven't read that part of the system. What I was seeing in my project (railpack build) was:
I can report back when I dig into this more and understand exactly where this is failing. |
5d86888
to
008efef
Compare
Ok, I discovered what was happening here: In the build script I was testing, I had a Automatically consuming
I disagree here:
Two other issues I found here:
I've updated the PR to correct these. My general thinking here is the provider/detection logic should be "magical" but any user-specified configuration inputs should be strict to guarantee deterministic build plans. |
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.
Automatically consuming railpack.json without a warning feels wrong to me. I think we should only read this during build if specified explicitly. Wdyt?
Can you expand on why it feels wrong? It is very standard across all tooling that supports a config file (Dockerfile, package.json, vite config, buildpacks).
Failing on an invalid railpack.json
file makes sense then. Agree it will help avoid confusion. In your case I recommend naming the file railpack-plan.json
core/core.go
Outdated
// unless an absolute path, assume relative to the app source directory | ||
var absConfigFileName string | ||
if !filepath.IsAbs(configFileName) { | ||
absConfigFileName = filepath.Join(app.Source, configFileName) |
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 this is too dangerous to do since it means that you can read a config file outside of the app directory. It is especially a problem since the config file path can come from a user defined env var
RAILPACK_CONFIG_FILE=/something/you/dont/have/access/to
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 can revert! I'll also update the docs/--help around this since it confused me.
Curious why that's a problem/security concern? If the user has access to those files during build, it doesn't seem risky to me.
On second thought, I'm wrong here. Most build tooling does automatically consume expected files, but most of the other CLI tools I use on a daily basis don't, which is why this felt wrong to me. I think it's worth adding |
That would make it much clearer! |
helpful if we expect the build to fail. There shouldn't be too many of these, but we'll need at least a couple to make sure cases like railpack.json syntax errors hard fail.
fc43ea2
to
352c3d3
Compare
I know basically nothing about Java and after I fixed the syntax error which was causing this config file to not be picked up, this started failing. The proper Zulu version check which is the main purpose of this is still working fine so I think it's fine to remove these custom commands.
@coffee-cup this one is ready for final review. |
I noticed this while debugging my project's build failure. I kept getting a message about malformed
railpack.json
even though I did not provide one directly. It seems as through the generatedrailpack.json
was incorrect but it did not cause the build to hard fail, it continued on without the generatedrailpack.json
.This change ensures that if invalid railpack.json is encountered, the CLI hard fails.
Breaking Change
Previously, if JSON did not conform to the schema it would just be ignored and the build would continue. With this change, the build will halt and force the user to fix or discard their schema. I prefer this more explicit approach.