Skip to content

Conversation

iocanel
Copy link
Contributor

@iocanel iocanel commented Oct 29, 2019

This pull request allows the user to generate knative manifests using the kubernetes extension.

It works in a similar manner with Openshift, just add:

kubernetes.deployment.target=knative

The pull request includes documentation changes and an integration test.

Note: The integration test is not as thorough as the rest of the kubernetes integration tests, as its blocked by fabric8io/kubernetes-client#1838 (only affects testing).

WIP flag added until dekorate 0.9.5 reaches maven central.

@geoand
Copy link
Contributor

geoand commented Oct 29, 2019

Cool!

@gsmet
Copy link
Member

gsmet commented Oct 29, 2019

I cancelled the build, we are scarce on CI resources right now.

@iocanel
Copy link
Contributor Author

iocanel commented Oct 29, 2019

Ok, I'll ping you when everything is in place.

@gsmet
Copy link
Member

gsmet commented Oct 29, 2019

OK, this will have to wait. Please do not push anything there today and tomorrow, we need the CI resources for the PRs that we need to get in.

(sorry about that but we are struggling a bit)

@gsmet gsmet added this to the 0.28.0 milestone Oct 29, 2019
@iocanel iocanel changed the title [wip] Add support for generating knative manifests using the kubernetes extension. Add support for generating knative manifests using the kubernetes extension. Oct 29, 2019
@iocanel
Copy link
Contributor Author

iocanel commented Oct 29, 2019

OK, this will have to wait. Please do not push anything there today and tomorrow, we need the CI resources for the PRs that we need to get in.

(sorry about that but we are struggling a bit)

@gsmet: Aplogies! didn't see your message in time and I pushed.

KubernetesList list = Serialization.unmarshal(knativeYml.text, KubernetesList.class)
assert list != null

// Un-comment once: https://github.com/fabric8io/kubernetes-client/issues/1838 is addressed.
Copy link
Contributor

Choose a reason for hiding this comment

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

But this won't affect the correctness of the generated resources, correct? It's just that with the bug in the client you can't convert the yml into the domain object, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly! The bug only affects deserialization which is soemthing not used by the extension. Its only needed for testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@geoand
Copy link
Contributor

geoand commented Oct 30, 2019

@iocanel would you like to update the PR?

@geoand
Copy link
Contributor

geoand commented Oct 31, 2019

Let's play it safe and keep this out of 0.28.0 for now.

@iocanel
Copy link
Contributor Author

iocanel commented Oct 31, 2019

@geoand: The pull request just adds dependencies, docs and tests.
It's as safe as it gets!

... famous last words

@geoand
Copy link
Contributor

geoand commented Oct 31, 2019

@iocanel I'll tell you what: If I have some time to do some extensive testing over the weekend, we can get it in.

At this point we are being very cautious so I would like to avoid any surprised :)

@gsmet gsmet removed this from the 0.28.0 milestone Oct 31, 2019
@gsmet
Copy link
Member

gsmet commented Nov 1, 2019

This will need a rebase as I renamed the doc file.

@gsmet gsmet added the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Nov 1, 2019
@iocanel
Copy link
Contributor Author

iocanel commented Nov 4, 2019

Rebased.

@geoand
Copy link
Contributor

geoand commented Nov 4, 2019

@iocanel does this also have to be updated to exclude the sundrio processors?

@iocanel
Copy link
Contributor Author

iocanel commented Nov 4, 2019 via email

@geoand
Copy link
Contributor

geoand commented Nov 5, 2019

Yes it needs to be updated or yes it has already been updated?

@gwenneg gwenneg removed the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Nov 8, 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.

Great stuff!

@geoand geoand merged commit 91bb7de into quarkusio:master Nov 11, 2019
@gsmet gsmet added this to the 1.1.0 milestone Nov 15, 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