console icon indicating copy to clipboard operation
console copied to clipboard

Update Devfile Import to use Kubernetes YAML definitions

Open maysunfaisal opened this issue 3 years ago • 8 comments
trafficstars

Description

This PR updates the Devfile handler backend code to consume the Kubernetes YAML file definitions. For now, the devfile samples only have the Deployment definition in the YAML file but the next course of action would be to update the Service, Route definitions in the devfile sample

Fixes https://github.com/devfile/api/issues/924

This PR just added the ability to read from Kubernetes YAML if the Devfile Kubernetes Component has inlined definition. If inlined is not present, it resorts to the previous way of generating Kubernetes resources. This should be compatible with previous OpenShift versions and the master branch

This PR also refactored some of the devfile backend code to keep it more clean and organized. Added tests to the devfile pkg since ODC didnt have go tests. Coverage %:

Screen Shot 2022-09-22 at 1 33 59 PM

How to test

(Check history for prev edit)

  • Devfile Import should work fine from the UI
  • If you are interested in testing the new approach, go to devfile-handler.go and update the following lines. Build and test it out by only importing a python devfile sample
         // Get devfile content and parse it using a library call in the future
	// devfileContentBytes := []byte(data.Devfile.DevfileContent)
	convert := true
	devfileObj, _, err = devfile.ParseDevfileAndValidate(parser.ParserArgs{
		// Data: devfileContentBytes,
		URL: "https://raw.githubusercontent.com/devfile-samples/devfile-sample-python-basic/87e269d5e018c904ba0a72b53292b3a9ed0b8b37/devfile.yaml",
		ConvertKubernetesContentInUri: &convert,
	})

maysunfaisal avatar Aug 29 '22 19:08 maysunfaisal

@yangcao77 could you PTAL from Devfile perspective?

maysunfaisal avatar Sep 22 '22 17:09 maysunfaisal

@jerolimov Thanks for the detailed testing and reports. The routes need to be defined in the devfile this way https://github.com/devfile-samples/devfile-sample-code-with-quarkus/pull/7/files#diff-23d39e3ae288b080d8aa0995928cd3c432f887a75f0ab3bba675089f51a4af8eR36-R39

One could also define routes in the YAML files but our official devfile samples will have the route definition in the component[x].kubernetes.endpoints

maysunfaisal avatar Oct 04 '22 18:10 maysunfaisal

@jerolimov Thanks for the detailed testing and reports. The routes need to be defined in the devfile this way https://github.com/devfile-samples/devfile-sample-code-with-quarkus/pull/7/files#diff-23d39e3ae288b080d8aa0995928cd3c432f887a75f0ab3bba675089f51a4af8eR36-R39

One could also define routes in the YAML files but our official devfile samples will have the route definition in the component[x].kubernetes.endpoints

Hi @maysunfaisal, I understand that it should be defined somewhere. But when its not defined the backend should not create broken resource definitions. I suggest that the Route is not exported in this case. Can you convert it to a pointer so that the JSON doesn't contain it anymore in this case?

christoph-jerolimov avatar Oct 11 '22 00:10 christoph-jerolimov

@jerolimov Thanks for the detailed testing and reports. The routes need to be defined in the devfile this way https://github.com/devfile-samples/devfile-sample-code-with-quarkus/pull/7/files#diff-23d39e3ae288b080d8aa0995928cd3c432f887a75f0ab3bba675089f51a4af8eR36-R39 One could also define routes in the YAML files but our official devfile samples will have the route definition in the component[x].kubernetes.endpoints

Hi @maysunfaisal, I understand that it should be defined somewhere. But when its not defined the backend should not create broken resource definitions. I suggest that the Route is not exported in this case. Can you convert it to a pointer so that the JSON doesn't contain it anymore in this case?

@maysunfaisal I agree with @jerolimov here that backend should not return a broken resource definition since the add flows will break. In case the route is not defined let's not return it. Please update the PR accordingly.

divyanshiGupta avatar Oct 11 '22 13:10 divyanshiGupta

@maysunfaisal I tested the UI PR with the current backend PR and it seems to work fine expect for the issue mentioned above. Resource URI is getting replaced with inline resource yaml and the api call is also successful with the new payload. All the desired resources are also returned but the route resource is broken as mentioned above.

https://user-images.githubusercontent.com/20724543/196163901-7499a168-7a35-4fe4-9ced-a139e584534c.mov

cc: @jerolimov @invincibleJai

divyanshiGupta avatar Oct 17 '22 11:10 divyanshiGupta

@divyanshiGupta @jerolimov I have rebased and addressed the PR concerns as well as our internal Slack thread conversation. PTAL.

I have tested 3 cases:

  1. Importing devfile sample without inline, it falls back to the current master branch approach. Successful.
  2. Importing devfile sample with inline including deployment, service and endpoint. Successful.
  3. Importing devfile sample with inline with only deployment, where service and endpoint are missing. Errors Out:
Screen Shot 2022-10-18 at 2 02 04 PM

maysunfaisal avatar Oct 18 '22 18:10 maysunfaisal

I observed one thing we should check on Slack. The Deployment contains a "variable" "{{CONTAINER_IMAGE}}" as image container.

@maysunfaisal It seems that this should be replaced with the image-name in the devfile. As @jerolimov mentioned its fine for phase 1 but lets improve it later and make sure all variables are replaced with their respective values.

divyanshiGupta avatar Oct 19 '22 09:10 divyanshiGupta

/label docs-approved /label px-approved

divyanshiGupta avatar Oct 19 '22 11:10 divyanshiGupta

/lgtm

christoph-jerolimov avatar Oct 20 '22 07:10 christoph-jerolimov

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerolimov, maysunfaisal

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci[bot] avatar Oct 20 '22 07:10 openshift-ci[bot]

Verified on cluster created on pr Screenshot from 2022-10-20 03-37-42 Screenshot from 2022-10-20 03-37-06 /label qe-approved

sanketpathak avatar Oct 20 '22 08:10 sanketpathak

/retest

divyanshiGupta avatar Oct 20 '22 12:10 divyanshiGupta

@maysunfaisal: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci[bot] avatar Oct 20 '22 16:10 openshift-ci[bot]