Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(trait): disable NodePort by default for Service trait #3263

Merged
merged 3 commits into from
May 12, 2022

Conversation

tadayosi
Copy link
Member

Fix #3253

Release Note

fix(trait): NodePort is disabled by default for Service trait

Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

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

Nice work. I think we can take the opportunity and add an E2E test in order to make sure this is not breaking in the future, wdyt?

@@ -125,6 +125,8 @@ func TestServiceWithDefaults(t *testing.T) {
assert.Len(t, d.Spec.Template.Spec.Containers[0].Ports, 1)
assert.Equal(t, int32(8080), d.Spec.Template.Spec.Containers[0].Ports[0].ContainerPort)
assert.Equal(t, "http", d.Spec.Template.Spec.Containers[0].Ports[0].Name)

assert.Empty(t, s.Spec.Type) // empty means ClusterIP
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for the clarifying comment

@tadayosi
Copy link
Member Author

@squakez Yes, having an E2E test is a good idea. I'm working on it.

@tadayosi
Copy link
Member Author

@squakez E2E test added.

@tadayosi
Copy link
Member Author

Failed E2E test is known flaky one and isn't related to this PR:

--- FAIL: TestHealthTrait (415.55s)
    --- FAIL: TestHealthTrait/Readiness_condition_with_stopped_route (408.02s)

Let me merge this.

@tadayosi tadayosi merged commit c19e4bf into apache:main May 12, 2022
@tadayosi tadayosi added the kind/bug Something isn't working label May 25, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable Service to be exposed as NodePort should be disabled by default
2 participants