Empty Aptfile Fix #118
Closed
Empty Aptfile Fix #118
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR fixes an error that occurs if an Aptfile with no packages is used. This case is detected now and a warning is printed advising that the buildpack and Aptfile should be removed before exiting the build. I've also added a test harness to this project with several variations of Aptfile contents we've seen used. There appears to have been tests in this repository at an earlier point but they were lost or abandoned at some point. This should prevent future accidental breakage if changes are needed for this buildpack.
This PR fixes an error that occurs if an Aptfile with no packages is used. This case is detected now and a warning is printed advising that the buildpack and Aptfile should be removed before exiting the build. I've also added a test harness to this project with several variations of Aptfile contents we've seen used. There appears to have been tests in this repository at an earlier point but they were lost or abandoned at some point. This should prevent future accidental breakage if changes are needed for this buildpack.
Merged
dzuelke
reviewed
Mar 19, 2024
|
|
||
| for PACKAGE in $(cat "$BUILD_DIR/Aptfile" | grep -v -s -e '^#' | grep -v -s -e "^:repo:"); do | ||
| while IFS= read -r PACKAGE; do | ||
| if [[ $PACKAGE == *deb ]]; then |
There was a problem hiding this comment.
While we're cleaning things up, this should be == *.deb ;)
| @@ -29,6 +29,17 @@ function indent() { | |||
| esac | |||
| } | |||
|
|
|||
| # exit early because we don't need to do any work if the aptfile has no packages | |||
There was a problem hiding this comment.
We're repeating these grep patterns a lot all over the place now.
I'd suggest doing this check a bit later, down before the for DEB in "$APT_CACHE_DIR/archives/"*.deb; do line, like this:
shopt -s nullglob
debs=( "$APT_CACHE_DIR/archives/"*.deb )
if ! (( ${#debs[@]} )); then
// no packages warning and exit 0
fi
for DEB in "${debs[@]}"; do
There was a problem hiding this comment.
I disagree. If there are no packages to install then we don't need to do any work including running apt-get update so I think the exit here makes more sense.
And that way, when packages are installed, the for DEB in "$APT_CACHE_DIR/archives/"*.deb line will not cause a failure so it can remain unchanged.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes an error that occurs if an
Aptfilewith no packages is used, that was introduced by #115. This case is detected now and a warning is printed advising that the buildpack andAptfileshould be removed before exiting the build.I've also added a test harness to this project with several variations of
Aptfilecontents we've seen used. There appears to have been tests in this repository at an earlier point but they were lost or abandoned at some point. This should prevent future accidental breakage if changes are needed for this buildpack.Fixes #117.