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

Create openshift wrapper extension. #7356

Merged
merged 3 commits into from Feb 27, 2020

Conversation

iocanel
Copy link
Contributor

@iocanel iocanel commented Feb 22, 2020

I want to make Quarkus on Openshift to be as easy as possible.

Right now the user needs:

  1. Add kubernetes extension
  2. Add container-image-s2i extension
  3. Set kubernetes.deploymmet.target=openshift

This pull request suggests an alternative aproach:

  1. Add openshift extension

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.

@iocanel
Copy link
Contributor Author

iocanel commented Feb 22, 2020

Just to clarify this is just a wrapper extension, all the functionality is still left to the existing kubernetes extension.

Copy link
Contributor

@geoand geoand left a 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

@geoand
Copy link
Contributor

geoand commented Feb 22, 2020

I personally like the idea a lot, it's definitely a +1 from me

@maxandersen
Copy link
Contributor

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.

@emmanuelbernard
Copy link
Member

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):

  • we must be able to use the underlying extensions to do the same
    • my main usage is for someone to be able to swap between Kube and OpenShift deployments
  • ideally an aggregator extension would just push "proposed" properties that are read by the config if it's not explicitly set by the user config. (but we don't have such framework today)
  • another way is via build items that should have less priority compared to an explicit property

So +1 to the idea.

@iocanel
Copy link
Contributor Author

iocanel commented Feb 24, 2020

  • another way is via build items that should have less priority compared to an explicit property

That's exactly the approach we've taken here.

@iocanel
Copy link
Contributor Author

iocanel commented Feb 24, 2020

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 ?

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:

  1. Certain APIS to be available on the target API server.
  2. A compatible container image to be available (e.g. s2i won't work with vanilla nor knative).

Now, if both of the conditions above are meet for more than one deployment target we should pickup the first one, unless the user has explicitly selected something else.

@iocanel iocanel force-pushed the openshift-extension branch 4 times, most recently from fa0a166 to 6293e32 Compare February 25, 2020 21:05
Copy link
Contributor

@geoand geoand left a 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

Copy link
Contributor

@geoand geoand left a 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 :)

docs/src/main/asciidoc/openshift.adoc Show resolved Hide resolved
docs/src/main/asciidoc/openshift.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/openshift.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/openshift.adoc Show resolved Hide resolved
docs/src/main/asciidoc/openshift.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/openshift.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/openshift.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/openshift.adoc Outdated Show resolved Hide resolved
@iocanel iocanel force-pushed the openshift-extension branch 2 times, most recently from a5163f9 to 0947b8d Compare February 26, 2020 08:43
@maxandersen
Copy link
Contributor

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

@iocanel iocanel force-pushed the openshift-extension branch 2 times, most recently from 59d15c6 to e79e775 Compare February 26, 2020 11:26
@geoand geoand changed the title [POC] Create openshift wrapper extension. Create openshift wrapper extension. Feb 26, 2020
@geoand
Copy link
Contributor

geoand commented Feb 26, 2020

Are we good to go with this?

@geoand geoand added this to the 1.3.0 milestone Feb 26, 2020
@iocanel
Copy link
Contributor Author

iocanel commented Feb 26, 2020

Are we good to go with this?

I think that everything is wrapped, so I'd say yeah.

@geoand
Copy link
Contributor

geoand commented Feb 26, 2020

I'll let @maxandersen push the magic button then

@maxandersen
Copy link
Contributor

Missed this. I'll try rebase it in a few hours when at desktop.

@geoand
Copy link
Contributor

geoand commented Feb 27, 2020

I rebased the PR onto master since Github UI was reporting a conflict

@maxandersen maxandersen merged commit 3aa36b5 into quarkusio:master Feb 27, 2020
@maxandersen
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants