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

Gh 212 #213

Merged
merged 3 commits into from
Feb 15, 2024
Merged

Gh 212 #213

merged 3 commits into from
Feb 15, 2024

Conversation

dmikusa
Copy link
Contributor

@dmikusa dmikusa commented Feb 11, 2024

Summary

Resolves #212

Use Cases

Make sure script is always executable.

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

Remove ioutil usage, and switch to using `t.Setenv`. Remove specific stacks in favor of wildcard stack.

Signed-off-by: Daniel Mikusa <dan@mikusa.com>
When the buildpack runs, it will locate the dist zip script to execute. If that script is found, then we also mark it as `0755`. This happens unconditionally, but we do not fail if there is an error. We just log a warning and proceed. It really shouldn't fail but maybe if the file had weird file permissions/ownership.

The choice to do this unconditionally is that it is slightly simpler. Checking and then setting is two operations. If thereis a preference to doing it that way, making the switch is easy enough. Just let me know

Signed-off-by: Daniel Mikusa <dan@mikusa.com>
@dmikusa dmikusa requested a review from a team as a code owner February 11, 2024 20:51
@dmikusa dmikusa added type:bug A general bug semver:patch A change requiring a patch version bump labels Feb 11, 2024
@anthonydahanne
Copy link
Member

Hello!
Could you test with the samples? in particular: dist-zip

Or maybe a new sample with startup.sh that is not executable?

@dmikusa
Copy link
Contributor Author

dmikusa commented Feb 12, 2024

The sample is difficult because it auto-generates the startup script. That means, it always generates it correctly with the right permissions. I did some manual testing with the sample app, but I had to build the ZIP, extract the files, change the permissions, make a new archive, and test with that.

I went a different route and covered this test case in the build_test.go with a test script that is generated by the test without the right permissions. It was easier to control and make the test predictable this way. I feel like that gives us pretty good coverage for this problem as well.

@anthonydahanne
Copy link
Member

I went a different route and covered this test case in the build_test.go with a test script that is generated by the test without the right permissions. It was easier to control and make the test predictable this way. I feel like that gives us pretty good coverage for this problem as well.

Yes, I see that, that's perfect - straight to the point! Thanks!

@anthonydahanne anthonydahanne merged commit 2758497 into main Feb 15, 2024
5 checks passed
@anthonydahanne anthonydahanne deleted the gh-212 branch February 15, 2024 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch A change requiring a patch version bump type:bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Application start script files execution permission is not checked or configured
2 participants