Skip to content

build_module.sh hardening & CI#117

Open
thresheek wants to merge 17 commits into
nginx:masterfrom
thresheek:buildmodulefixes
Open

build_module.sh hardening & CI#117
thresheek wants to merge 17 commits into
nginx:masterfrom
thresheek:buildmodulefixes

Conversation

@thresheek

Copy link
Copy Markdown
Member

Proposed changes

This PR attempts to make build_module.sh less of a mess and adds (some) CI to make sure it's not broken.

@thresheek thresheek requested a review from a team as a code owner June 3, 2026 01:31
@thresheek thresheek requested a review from Copilot June 3, 2026 01:39

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens build_module.sh to be safer and more robust (quoting, temp-dir handling, URL download retries, better errors) and introduces a GitHub Actions workflow to exercise the script in CI across a small OS/NGINX flavor matrix.

Changes:

  • Hardened build_module.sh with better quoting, safer temp directory creation, and clearer error handling around git/dir operations.
  • Added a retrying URL download helper and basic archive path-safety checks for zip/tar sources.
  • Added a new GitHub Actions workflow to run build_module.sh in containers for OSS and Plus builds.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 10 comments.

File Description
build_module.sh Refactors/hardens argument handling, downloading, temp build dir creation, and packaging artifact collection.
.github/workflows/build-module.yml Adds CI job to build a sample module across OS/flavor combinations.
Comments suppressed due to low confidence (1)

build_module.sh:130

  • The -v flag allows an optional OSS version, but the minimum-version check runs even when no version was provided. With -v <module_source> (no explicit version), OSS_VER remains unset/empty and the comparison treats it as less than 1.11.5, causing an unconditional error. Gate the version comparison behind a non-empty OSS_VER (or set a safe default) so -v without an explicit version works as intended.
			if [ `printf '%s' ".$2" | tr -d '[0-9\.]' | wc -c` -eq 0 ]; then
				OSS_VER=$2
				shift
			fi
			if [ `echo "1.11.4^$OSS_VER" | tr '^' '\n' | tr '.' ',' | sort -nr | head -1` = "1,11,4" ]; then

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread build_module.sh Outdated
Comment thread build_module.sh Outdated
Comment thread build_module.sh
Comment thread build_module.sh Outdated
Comment thread build_module.sh
Comment thread build_module.sh Outdated
Comment thread build_module.sh
Comment thread .github/workflows/build-module.yml
Comment thread .github/workflows/build-module.yml
Comment thread build_module.sh
Comment thread build_module.sh
PACKAGE_SOURCES_DIR=extra
PACKAGE_OUTPUT_DIR="debuild-module-*/"
elif [ `apk --version | grep -c "^apk-tools"` -eq 1 ]; then
elif [ `apk --version 2>/dev/null | grep -c "^apk-tools"` -eq 1 ]; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jftr: the whole approach "lets find some package managers" doesn't work that well. It seems like most modern OS use /etc/os-release which we can source, find ID and use that to determine the package manager.

@oxpa

oxpa commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

I don't quite get the benefit of "printf "%s" over "echo -n" though. But looks good anyways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants