-
Notifications
You must be signed in to change notification settings - Fork 137
Use relative module path for both UBI and Alpine builds #4004
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
Conversation
Add bundle, update RBAC, use root helm chart
Add bundle, update RBAC, use root helm chart
Update READMEs and Makefiles to provide instructions for running the conformance tests on OpenShift clusters, and easy targets for same
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/openshift-support #4004 +/- ##
==========================================================
+ Coverage 86.77% 86.80% +0.03%
==========================================================
Files 128 128
Lines 16607 16607
Branches 62 62
==========================================================
+ Hits 14410 14416 +6
+ Misses 2012 2008 -4
+ Partials 185 183 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
||
# Configure directories and logging | ||
RUN mkdir -p /usr/lib/nginx/modules /var/run/nginx /usr/lib64/nginx/modules \ | ||
RUN mkdir -p /var/run/nginx /usr/lib64/nginx/modules \ |
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.
why not to make /usr/lib64/nginx/modules
env variable?
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 don't quite understand.
If we set /usr/lib64/nginx/modules
as an env variable (e.g. export LIB_DIR=/usr/lib64/nginx/modules
) does that mean we can avoid making the directory?
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.
no, just having it to edit in one place
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 don't think it should be an ENV var because then it's technically over writable at build time which is not what we want here. It's correct to be explicit here - and anyway it's only specified in the file twice.
tests/go.mod
Outdated
replace github.com/nginx/nginx-gateway-fabric/v2 => ../ | ||
|
||
require ( | ||
github.com/nginx/nginx-gateway-fabric/v2 v2.1.1 |
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'll rebase the base branch to main again so that we don't need these changes here
4504014
to
28d8e3f
Compare
Proposed changes
This change updates references to
/usr/lib/nginx/modues
to use the relative pathmodules/<module_to_load>
By default NGINX creates a symlink from
/etc/nginx/modules/
to the appropriate module dir based on the base image usedCloses #4002
Checklist
Before creating a PR, run through this checklist and mark each as complete.
Release notes
If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.