Skip to content
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

Fix old tag of custom error pages used in example #7460

Conversation

DysphoricUnicorn
Copy link
Contributor

What this PR does / why we need it:

The 0.4 update fixed an issue (#4039), which caused the error pages to be incorrectly displayed (or in the case of chromium not at all displayed) by web browsers.

Since I didn't check which tag is the current one, I actually wrote a fix for this issue, only to see that it was already fixed in 0.4 :/
I just updated the tag in the example manifest so that others won't run into the same issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

How Has This Been Tested?

I deployed the updated manifest to my production cluster and it works

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have added tests to cover my changes. -> does not apply
  • All new and existing tests passed. -> does not apply

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 7, 2021
@k8s-ci-robot
Copy link
Contributor

@DysphoricUnicorn: This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added the needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Aug 7, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @DysphoricUnicorn. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority area/docs labels Aug 7, 2021
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 7, 2021
Copy link
Member

@tao12345666333 tao12345666333 left a 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
/trigger accept

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 8, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 8, 2021
@rikatz
Copy link
Contributor

rikatz commented Aug 8, 2021

Wondering if we should move this image to kubernetes repo as well (as I don’t know who have access to this quay.io repo)

@tao12345666333
Copy link
Member

If we want to move it to kubernetes repo, we can just sync the image tags 😺

@DysphoricUnicorn
Copy link
Contributor Author

Is there anything I can do to help with that?

I know, it's not a high priority change but if I can stop some other newcomer from running into this issue like I did, I definitely want to.

@rikatz
Copy link
Contributor

rikatz commented Aug 9, 2021

@tao12345666333 didn't get your idea :)

Was going to suggest to @DysphoricUnicorn to follow the same approach of other images: https://github.com/kubernetes/ingress-nginx/tree/main/images/httpbin

  • Create a simple makefile to compile it
  • Copy some cloudbuild.yaml from other images

Once this gets merged, probably (see, probably because I don't know!!) cloudbuild would compile this image in staging and we can promote it.

Actually this repo could make some good usage of a cleanup of everything outside k8s.gcr.io :)

But coming back to this PR:

@DysphoricUnicorn take a look into https://github.com/kubernetes/ingress-nginx/tree/main/images/httpbin, if you can create a really simple Makefile, to be called by cloudbuilds.yaml we can work on moving this to gcr :)

Since the setup for the custom-error-messages was really different from
the other images that are build using cloudbuild, I changed it to "fit
in better"
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 9, 2021
@DysphoricUnicorn
Copy link
Contributor Author

I updated the Makefile and added a cloudbuild.yml

I don't actually know enough about cloudbuild to verify the changes though.
When I had just updated the Makefile, the docker build would fail for me and since the way it was built looked way different from everything else, I updated it to "fit in better".

I did so to the best of my current knowledge and similar to how I build dockerfiles at my dayjob.
This may clash with the common practices for K8S projects and if there are any issues, I am happy to resolve them.

@tao12345666333
Copy link
Member

@tao12345666333 didn't get your idea :)

@rikatz I mean, we can directly use tools like https://github.com/containers/skopeo to synchronize the original images to the new repo

@DysphoricUnicorn
Copy link
Contributor Author

Did you get a chance to look at my changes yet?

Since the automated build jobs succeeded (as well as my local test builds that I did before pushing) they are probably correct but if they aren't, I'll be glad to update the pr again.

@rikatz
Copy link
Contributor

rikatz commented Aug 21, 2021

@DysphoricUnicorn adding this to milestone v1.0.1 to take a proper look
/priority important-soon

@k8s-ci-robot k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Aug 21, 2021
@rikatz rikatz added this to the v1.0.1 milestone Aug 21, 2021
@@ -12,10 +12,23 @@
# See the License for the specific language governing permissions and
# limitations under the License.

FROM BASEIMAGE
FROM golang:1.14-alpine as builder
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use go v1.17 here?

@rikatz
Copy link
Contributor

rikatz commented Sep 7, 2021

overall lgtm.

We can add this image and later add it to the test-infra and promotion infra.

@DysphoricUnicorn you can do that:

Feel free to ping me in Slack with further questions (I keep losing github notifications...)

Since Go >= 1.16 required the use of modules, I also initialized the module using the name k8s.io/ingress-nginx/custom-error-pages
@@ -36,7 +36,7 @@ spec:
spec:
containers:
- name: nginx-error-server
image: quay.io/kubernetes-ingress-controller/custom-error-pages-amd64:0.3
image: gcr.io/k8s-staging-ingress-nginx/nginx-errors:0.48.1
Copy link
Contributor

Choose a reason for hiding this comment

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

One last change:

The image path should be k8s.gcr.io/ingress-nginx/nginx-errors:0.48.1

Anyway, I'm ok to fix this later as we promote the image :)

@rikatz
Copy link
Contributor

rikatz commented Sep 9, 2021

Gonna approve and merge this.

@DysphoricUnicorn as soon as this gets merged, gonna send the right tag here so you can open a PR for image promotion and fixing the customization file here.

/approve
/lgtm
Thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 9, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DysphoricUnicorn, rikatz, tao12345666333

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 9, 2021
@k8s-ci-robot k8s-ci-robot merged commit c9a00fb into kubernetes:main Sep 9, 2021
rchshld pushed a commit to joomcode/ingress-nginx that referenced this pull request May 19, 2023
* Fix old tag of custom error pages used in example

* Move nginx-errors to k8s registry

Since the setup for the custom-error-messages was really different from
the other images that are build using cloudbuild, I changed it to "fit
in better"

* Use Go version 1.17 for custom-error-pages

Since Go >= 1.16 required the use of modules, I also initialized the module using the name k8s.io/ingress-nginx/custom-error-pages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/docs cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants