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

This buildpack can now use the tiny stack with reloadable processes #311

Conversation

joshuatcasey
Copy link
Contributor

Summary

This buildpack can now use the tiny stack with reloadable processes. While #296 addressed this, there was some logic in the Detect method that was not removed.

See #296 (comment) for additional context.

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).

@joshuatcasey joshuatcasey requested a review from a team as a code owner May 9, 2022 15:34
@joshuatcasey joshuatcasey self-assigned this May 9, 2022
@joshuatcasey joshuatcasey added the semver:patch A change requiring a patch version bump label May 9, 2022
@fg-j
Copy link

fg-j commented May 9, 2022

I think the integration test is failing because of the WithPullPolicy("never"). The builder isn't present on the GHA runner docker daemon.

@joshuatcasey
Copy link
Contributor Author

Good catch @fg-j . "It worked on my machine!"

Will push a fix in a minute.

@joshuatcasey
Copy link
Contributor Author

@fg-j it looks like using the tiny builder is causing further integration test problems. Should I pull it out?

I admit that this integration test would have caught the logic in Detect that this PR fixes, but I feel like this PR addresses that and so there's little need to make sure an integration test verifies this.

@fg-j
Copy link

fg-j commented May 10, 2022

Hmm... I'm concerned that that integration test isn't passing. It should be, if the buildpack actually works with watchexec. This warrants more investigation.

@ForestEckhardt
Copy link
Contributor

Grabbed the app locally and I get the following error

[[Error (not fatal)]]
io(spawning process group): No such file or directory (os error 2)

@joshuatcasey joshuatcasey force-pushed the jtc/tiny-stack-and-watchexec branch from baaf184 to f30404a Compare May 12, 2022 04:24
@joshuatcasey
Copy link
Contributor Author

I've completely restored the integration test to its original state. Even though this has passed on my machine it's pretty flaky even locally.

I wonder if there's some nuance of how the builder is pulled or used that may be an issue. A quick grep through paketo codebases indicates that integration tests don't usually set the builder, although smoke tests often do (for samples and builders).

@fg-j
Copy link

fg-j commented May 12, 2022

@joshuatcasey You might get a clearer idea of what's failing in the container on startup if you turn up the verbosity on watchexec
See the watchexec man page

- Also pull the tiny builder and tiny run image for integration tests
@joshuatcasey
Copy link
Contributor Author

@ForestEckhardt would you mind taking a look at the PR? I think we've gotten past the integration test issue.

@ForestEckhardt ForestEckhardt merged commit 9e21717 into paketo-buildpacks:main May 17, 2022
@joshuatcasey joshuatcasey deleted the jtc/tiny-stack-and-watchexec branch May 17, 2022 14:50
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
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants