-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
NGINX: Add NJS. #12345
base: main
Are you sure you want to change the base?
NGINX: Add NJS. #12345
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: anurag-rajawat The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @anurag-rajawat! |
Hi @anurag-rajawat. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
✅ Deploy Preview for kubernetes-ingress-nginx ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
a435869
to
dce4341
Compare
Great start. Can you add an e2e test that exercises using the njs model? |
This may have to wait till we release a new nignx base image. We do rebuild the controller in all e2e but not the nginx base. |
/ok-to-test |
/kind feature |
We do build the nginx image, so we can add some e2e test for njs. |
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'm not even sure if we should still add this on Ingress NGINX. @strongjz
/hold
Hey @strongjz, should we proceed with further changes? |
Let's chat after our maintainers talk tomorrow. We've got plans for things going forward. |
Also need to rebase to get the go lint ci 0207d18 |
@Gacko I am not following the relation here. Is main branch broken, or are you cutting 1.12 from a release branch? What is broken? What is the status of the fix? |
## enable-njs | ||
Enables or disables [njs](https://nginx.org/en/docs/njs/) module. _**default: false**_ |
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.
## enable-njs | |
Enables or disables [njs](https://nginx.org/en/docs/njs/) module. _**default: false**_ | |
## enable-njs | |
Enables or disables [NJS](https://nginx.org/en/docs/njs) module. _**default: false**_ |
Every branch is broken because we are doing an |
Specifically what is broken from the upgrade? Which part of the build? |
There are compilation failures, I'm already working on this. You can see the Just keep in mind: What ever you change, we won't release it before v1.13 anyway as there already is a v1.12 on its way and we first need to fix this. So new features like this, cross plane or NGINX v1.27.1 are not urgent and can stay unmerged for now. |
The idea is not to release on v1.12 any of crossplane or njs changes, I was more willing to not block changes that long and trying to understand what's going on the build. I will take a look into your branch and recent cloud builds to understand what's going on |
Of course we are not planning to release Crossplane or NJS on v1.12, but currently the NGINX base image Cloud Build is broken on all the branches and I'd like to prevent any changes to the NGINX base image before we haven't figured out what's wrong, also because we normally build the NGINX base image for all branches from the Ideally we are building the NGINX base image per branch, but this is something we need to figure out via well thought release engineering and versions in the TAG file across branches. |
be3b798
to
120275a
Compare
120275a
to
c00733a
Compare
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.
Requesting changes so this isn't getting merged, yet, as we are currently still working on fixing the NGINX build first.
c00733a
to
96149f5
Compare
Signed-off-by: Anurag Rajawat <[email protected]>
96149f5
to
d577f98
Compare
NGINX build is working again. We're now releasing v1.12 and then coming back here. |
This is a duplicate of https://github.com/kubernetes/ingress-nginx/pull/12447/files so need to close this |
No, it isn't. The other PR is newer and based on this PR. |
Can we track the change proposed in the other one instead of this one ? |
No, I'd like to keep this one as it's really only introducing NJS and this should complete the tests on its own. |
ok, then that implies a comment on the other one to remove the duplicacy |
The other one needs to be based on this here so it can already be tested. The moment we merge this one, @elizabeth-dev will rebase their PR. |
👍 left a note there |
yes, my PR(s) already start from this PR's branch, so it'll be an easy rebase when this is merged |
What this PR does / why we need it:
This PR is the continuation of #11248.
Types of changes
Which issue/s this PR fixes
How Has This Been Tested?
I verified my changes by:
Here is a screenshots that show it has
nginx-njs
module:Checklist: