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(service): Create k8s service when knative-service trait is disabled #3871

Merged
merged 1 commit into from
Dec 5, 2022

Conversation

claudio4j
Copy link
Contributor

#3849

When knative-service and k8s service traits are enabled, the priority is to use knative-service in knative profile

Release Note

NONE

Copy link
Member

@tadayosi tadayosi left a comment

Choose a reason for hiding this comment

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

I'm still not sure if it's the right direction. Could you answer to my question at #3849?

pkg/trait/service.go Outdated Show resolved Hide resolved
e2e/global/knative/knative_test.go Show resolved Hide resolved
e2e/global/knative/knative_test.go Show resolved Hide resolved
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.

I've reviewed the backport one (in the future, I'd prefer we wait to provide a backport PR until the main one has been approved/discussed). They are the same consideration for this #3870 (review)

@christophd
Copy link
Contributor

uff, that confused me as well. I also added some review comments to the backport PR. Funnily enough the comments are quite the same as here 😄

@claudio4j
Copy link
Contributor Author

claudio4j commented Dec 2, 2022

uff, that confused me as well. I also added some review comments to the backport PR. Funnily enough the comments are quite the same as here smile

Oh, that's not good from my PR, I did this in a hurry. Good reminder for next PR.

@claudio4j
Copy link
Contributor Author

/retest

Copy link
Member

@tadayosi tadayosi left a comment

Choose a reason for hiding this comment

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

Request change for the license header.

e2e/global/knative/files/http_out.groovy Show resolved Hide resolved
e2e/global/knative/knative_test.go Show resolved Hide resolved
e2e/global/knative/knative_test.go Show resolved Hide resolved
…abled

apache#3849

When knative-service and k8s service traits are enabled, the priority is
to use knative-service in knative profile
@christophd christophd mentioned this pull request Dec 5, 2022
@claudio4j claudio4j merged commit a82e464 into apache:main Dec 5, 2022
@claudio4j claudio4j deleted the fix_knativeservice_main branch December 5, 2022 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants