-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Create openshift wrapper extension. #7356
Conversation
88d1d50
to
1c64f47
Compare
Just to clarify this is just a wrapper extension, all the functionality is still left to the existing kubernetes extension. |
1c64f47
to
7dd19ad
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.
I added a comment about the code gen that I don't really understand
extensions/kubernetes/openshift/deployment/src/main/resources/config-wrapper.vm
Outdated
Show resolved
Hide resolved
I personally like the idea a lot, it's definitely a +1 from me |
I like this idea of 'starter-extensions' too; wondering if the approach has issues if we start have more and more of these - what happens if you add a openshift and a knative starter - which one wins ? /cc @emmanuelbernard @tqvarnst for awareness as we discussed these things in past of various degrees. |
...s/vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/KubernetesProcessor.java
Show resolved
Hide resolved
I think it makes a lot of sense. I knew at some point some umbrella extension would just profile / tune the usage of other extensions. So +1 to me. A few general comments though (I did not look at the implementation):
So +1 to the idea. |
That's exactly the approach we've taken here. |
7dd19ad
to
5f620b5
Compare
Thats' s an excellent question! Here are some thoughts: In order to be able to perform a deployment for any platform, we have the following requirements:
Now, if both of the conditions above are meet for more than one |
fa0a166
to
6293e32
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.
Added a few comments and questions
...s/vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/KubernetesProcessor.java
Outdated
Show resolved
Hide resolved
.../vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/KubernetesConfigUtil.java
Outdated
Show resolved
Hide resolved
...kubernetes/openshift/deployment/src/main/java/io/quarkus/openshift/deployment/Constants.java
Show resolved
Hide resolved
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.
And some comments on the doc :)
a5163f9
to
0947b8d
Compare
looks really good - much simpler story in docs too. One nice addition would be to add labels to the Deployment/Deployment Config so it will get Quarkus icon in dev console. needs to have either app.openshift.io/runtime=quarkus or app.kubernetes.io/name=quarkus label. details at openshift/console#4306 |
59d15c6
to
e79e775
Compare
Are we good to go with this? |
I think that everything is wrapped, so I'd say yeah. |
I'll let @maxandersen push the magic button then |
Missed this. I'll try rebase it in a few hours when at desktop. |
e79e775
to
623f3f9
Compare
I rebased the PR onto master since Github UI was reporting a conflict |
extensions/kubernetes/vanilla/runtime/src/main/resources/META-INF/quarkus-extension.yaml
Outdated
Show resolved
Hide resolved
623f3f9
to
350cc67
Compare
350cc67
to
c6bcc8c
Compare
one open question is the status of the extension is marked stable even thuogh its new and depends on container-image that are preview. ill open separate issue for that. |
I want to make
Quarkus on Openshift
to be as easy as possible.Right now the user needs:
kubernetes.deploymmet.target=openshift
This pull request suggests an alternative aproach:
In other words, it provides a wrapper extension the makes it possible to deploy to openshift without additional configuration, while at the same time maintaining backwards compatibility.