Skip to content

Conversation

iocanel
Copy link
Contributor

@iocanel iocanel commented Oct 27, 2019

This pull request brings Openshift support to the kubernetes-extension.

The idea is that the user is able to select among:

  • kubernetes
  • openshift
  • both

using an option like:

kubernetes.deployment.target=openshift in the application.properties.

The pull request includes integration-test and docs and also a tiny fix to the existing integration tests.

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.

Looks good 👍!

Assuming integration tests pass, let's merge

@geoand geoand added release/noteworthy-feature triage/waiting-for-ci Ready to merge when CI successfully finishes labels Oct 27, 2019
@geoand geoand added this to the 0.27.0 milestone Oct 27, 2019
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.

Just some minor comments. I think we can get them in once addressed


=== Openshift support

To enable the genration of Openshift resources, you need to include Openshift in the target platforms:
Copy link
Contributor

Choose a reason for hiding this comment

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

genration -> generation


[source]
----
kubernetes.deployment.target=kubernetes openshift
Copy link
Contributor

@geoand geoand Oct 27, 2019

Choose a reason for hiding this comment

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

Is this whitespace separated or comma separated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whitespace

Copy link
Collaborator

Choose a reason for hiding this comment

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

We use comma's everywhere else, it should also use comma's for consistency.

Is there any reason why this is not using the Quarkus configuration model, but is doing its own config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to comma separated.

Regarding the config, this pull request does get the config from the Quarkus ConfigProvider and converts it to the dekorate format. Does this anser your question @stuartwdouglas or you meant something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

@stuartwdouglas The configuration of this extension uses the pass-through strategy like the Kafka Streams does.
Originally this extension had the regular Quarkus configuration but it was too limiting so it was switched as part of #4308 (see this for more details)

@geoand geoand removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 27, 2019
kubernetes.deployment.target=openshift
----

If you need to generate resources for both platforms (vanilla kubernetes and opesnhift), then you need to include both (space separated).
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I just saw that Openshift is spelled wrong here.


[source]
----
kubernetes.deployment.target=kubernetes openshift
Copy link
Collaborator

Choose a reason for hiding this comment

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

We use comma's everywhere else, it should also use comma's for consistency.

Is there any reason why this is not using the Quarkus configuration model, but is doing its own config?

@gsmet
Copy link
Member

gsmet commented Oct 28, 2019

Please make the title a bit more descriptive of what this PR is about as it will end up in the announcement email. Thanks!

@iocanel iocanel changed the title Openshift support Support generating Openshift manifests Oct 28, 2019
@geoand
Copy link
Contributor

geoand commented Oct 28, 2019

@stuartwdouglas Have your comments been addressed?

@stuartwdouglas stuartwdouglas merged commit 576048a into quarkusio:master Oct 28, 2019
@gsmet gsmet changed the title Support generating Openshift manifests Support generating OpenShift manifests Oct 30, 2019
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.

4 participants