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.

Sign up for GitHub

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

Jump to bottom

Add conformance echo-server code and image build (echo-basic) #2447

Merged
merged 13 commits into from
Oct 5, 2023
Merged

Add conformance echo-server code and image build (echo-basic) #2447

merged 13 commits into from
Oct 5, 2023

Conversation

SRodi
Copy link
Member

@SRodi SRodi commented Sep 30, 2023 •
edited
Loading

What type of PR is this?

/kind test
/area conformance

What this PR does / why we need it:
Add codebase and image build step for echoserver image used in conformance tests. Prevent to update https://github.com/kubernetes-sigs/ingress-controller-conformance when we need to tweak base image for conformance.

Which issue(s) this PR fixes:

Fixes #2445

Does this PR introduce a user-facing change?:

NONE

add echo-basic (previously known as echo-server) from Ingress Controller Conformance Repo

@SRodi
add-echo-server-code
2e1e2ae 
* add code from Ingress Controller Conformance Repo
* update build-and-push.sh to also build this image
* create Dockerfile.echoserver
* add go.work at root with reference to ./conformance/echoserver
* add delay support to echoserver
* relates to kubernetes-sigs#2445
* discussed in kubernetes-sigs#2254
@k8s-ci-robot k8s-ci-robot added kind/test area/conformance do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 30, 2023
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 30, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @SRodi. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 30, 2023
@SRodi
Copy link
Member Author

SRodi commented Sep 30, 2023

@robscott @frankbu please take a look at this PR. We will also need a followup PR once the image is built, to update the images used in conformance tests.

@SRodi SRodi changed the title add-echo-server-code Add conformance echo-server code and image build Sep 30, 2023
frankbu
frankbu reviewed Oct 2, 2023
View reviewed changes
robscott
robscott reviewed Oct 2, 2023
View reviewed changes
Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Thanks for the work on this @SRodi!

@youngnick youngnick mentioned this pull request Oct 3, 2023
Open
@SRodi
add-echo-basic
54a1b95 
* rename to echo-basic (Istio's one can be echo-advanced)
* up go.mod go version to 1.20
* fix dockerfile golang version and COPY instruction (paths)
* update build-and-push.sh and go.work with new namings
@SRodi SRodi requested review from dprotaso, frankbu and robscott October 3, 2023 05:31
@SRodi SRodi changed the title Add conformance echo-server code and image build Add conformance echo-server code and image build (echo-basic) Oct 3, 2023
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 3, 2023
@frankbu
Copy link
Contributor

frankbu commented Oct 3, 2023

@robscott @frankbu please take a look at this PR. We will also need a followup PR once the image is built, to update the images used in conformance tests.

@SRodi as soon as this merges, I can update #2254 to use it for one service, sort of a canary test 😃 If that works out, then we can do a followup PR to change the rest of the services to use the new image.

@SRodi
Copy link
Member Author

SRodi commented Oct 3, 2023

@SRodi as soon as this merges, I can update #2254 to use it for one service, sort of a canary test 😃 If that works out, then we can do a followup PR to change the rest of the services to use the new image.

@frankbu that sounds good. I will keep #2437 open (as a draft for now) so that I can proceed with the general image update once you confirm your "canary test" works.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 4, 2023
robscott
robscott reviewed Oct 4, 2023
View reviewed changes
Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Thanks @SRodi!

/ok-to-test

go.work
@@ -0,0 +1,6 @@
go 1.20
Copy link
Member

Choose a reason for hiding this comment

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

Although I can't find any absolute guidance, it appears that the most common advice seems to be to add this and go.work.sum to .gitignore per golang/go#53502. I'd recommend doing that for now and we can revisit adding these files at a later point.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tests are failing on prow if I remove go.work.sum and go.work. The message is: conformance/echo-basic does not contain main module https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_gateway-api/2447/pull-gateway-api-test/1709897756849475584 -

Can we include go.work.sum and go.work for now or do you prefer that I add its own go.mod in conformance/echo-basic ? @robscott

Copy link
Member

Choose a reason for hiding this comment

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

Good to know, thanks for giving it a try!

@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 Oct 4, 2023
@robscott robscott added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 4, 2023
@SRodi SRodi requested a review from robscott October 5, 2023 11:47
@robscott
Copy link
Member

robscott commented Oct 5, 2023

Thanks @SRodi!

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: robscott, SRodi

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 Oct 5, 2023
@k8s-ci-robot k8s-ci-robot merged commit 38125d1 into kubernetes-sigs:main Oct 5, 2023
@SRodi SRodi deleted the add-echo-server-code branch October 5, 2023 20:33
@SRodi SRodi mentioned this pull request Oct 6, 2023
Closed
9 tasks
@robscott robscott mentioned this pull request Oct 10, 2023
Merged
@robscott robscott mentioned this pull request Oct 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@howardjohn howardjohn Awaiting requested review from howardjohn

@youngnick youngnick Awaiting requested review from youngnick

@dprotaso dprotaso Awaiting requested review from dprotaso

@frankbu frankbu Awaiting requested review from frankbu

@robscott robscott Awaiting requested review from robscott

Assignees

@robscott robscott

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/test lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Conformance Echo Server Depends on Ingress Controller Conformance Repo
6 participants
@SRodi @k8s-ci-robot @frankbu @robscott @dprotaso @youngnick