-
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
fixes brotli build issue #10484
fixes brotli build issue #10484
Conversation
✅ Deploy Preview for kubernetes-ingress-nginx canceled.
|
This issue is currently awaiting triage. If Ingress contributors determines this is a relevant issue, they will accept it by applying the The 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/test-infra repository. |
Welcome @u5surf! |
Hi @u5surf. 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/test-infra repository. |
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.
/ok-to-test
/lgtm
Thanks |
/hold Wait for @strongjz |
can you rebase, we added a new test that does the nginx compile in GHA, if that passes then we can accept this PR. |
8bded91
to
943b7c8
Compare
All tests have passed. I think we can merge this. cc @strongjz |
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.
/approve
/lgtm
943b7c8
to
adb7465
Compare
Thank you for your contributions. The brotil build has been fixed in #10668. I will do the rebase and we can decide if we need to change the depth.
|
adb7465
to
f1da3a0
Compare
images/nginx/rootfs/build.sh
Outdated
@@ -465,7 +465,7 @@ make install | |||
|
|||
# Get Brotli source and deps | |||
cd "$BUILD_PATH" | |||
git clone --depth=100 https://github.com/google/ngx_brotli.git | |||
git clone --depth=1 https://github.com/google/ngx_brotli.git | |||
cd ngx_brotli | |||
# https://github.com/google/ngx_brotli/issues/156 | |||
git reset --hard 63ca02abdcf79c9e788d2eedcc388d2335902e52 |
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.
Due to the previous modification being reverted,
#10697
this fix still exists here.
Can you remove lines 470-471 so that we can use --depth=1
Thanks!
f1da3a0
to
9c7de68
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.
/hold cancle
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cpanato, tao12345666333, u5surf The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/cherry-pick release-1.10 |
@Gacko: new pull request created: #11187 In response to this:
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/test-infra repository. |
What this PR does / why we need it:
brotli’s build issue had fixed a few minuites ago. Thus, we should revert the build script.
google/ngx_brotli#156
Types of changes
Which issue/s this PR fixes
How Has This Been Tested?
Checklist: