Update Devfile Import to use Kubernetes YAML definitions#12000
Update Devfile Import to use Kubernetes YAML definitions#12000openshift-merge-robot merged 9 commits intoopenshift:masterfrom
Conversation
pkg/server/devfile-handler.go
Outdated
There was a problem hiding this comment.
testingutil sounds like a unit test utility, is this really how it should work?
pkg/server/devfile-handler.go
Outdated
There was a problem hiding this comment.
I understand that this is WIP, but as a reminder that we don't want merge this URL unexpectedly.
c07f5b2 to
2fcc67e
Compare
2fcc67e to
f1d0fd8
Compare
|
@yangcao77 could you PTAL from Devfile perspective? |
yangcao77
left a comment
There was a problem hiding this comment.
generally looks good from devfile perspective. a couple suggestions added
pkg/devfile/utils.go
Outdated
There was a problem hiding this comment.
can simply check if the imageBuildComponent has been set. so you don't need to introduce a count. and it does not need to iterate through the entire imageComponent list if it found an additional imagecomponent. (can save a little space & time :-D)
| imageDeployComponentCount++ | |
| if reflect.DeepEqual(imageBuildComponent, devfilev1.Component{}) { | |
| imageBuildComponent = component | |
| } else { | |
| // return error | |
| } |
and in the end, if the imageBuildComponent is unset, error out as well.
pkg/devfile/resources.go
Outdated
There was a problem hiding this comment.
it was already in master branch, I didnt introduce this; I just merely moved the code. But let me see what i can do
meant to post it in another comment
pkg/devfile/resources.go
Outdated
There was a problem hiding this comment.
why we want to pass in the imageComponentFilter as an arg? can this be defined within GetService?
There was a problem hiding this comment.
it was already in master branch, I didnt introduce this; I just merely moved the code. But let me see what i can do
pkg/devfile/resources.go
Outdated
There was a problem hiding this comment.
instead of a if-else statement. can check if component.Kubernetes.Inlined == "", and do the dev preview way. since it's going to break out if falls in that if statement. the src definition can be defined with all the stuffs starting at line 186
pkg/devfile/resources.go
Outdated
There was a problem hiding this comment.
should not use an internal endpoint to create route neither
pkg/devfile/resources.go
Outdated
There was a problem hiding this comment.
you don't need to check for this. it's been done in parser already https://github.com/kim-tsao/library/blob/4c7c5dc73eee13b3db599a7e5fd2b266420cd390/pkg/devfile/parser/parse.go#L702
There was a problem hiding this comment.
I would still need to check if its nil because I am accessing it like *endpoint.Secure; I didnt check initially and it panicked in one of my tests with an empty secure
There was a problem hiding this comment.
in unit test, yes, you can assign a nil to endpoint.Secure. but in actual case, all boolean values will be set before returning devfileObj
But if you think it's safer to check & assign in ODC code, I'm good with that.
pkg/devfile/resources.go
Outdated
There was a problem hiding this comment.
instead of doing the concatenating. can use github.com/hashicorp/go-multierror?
and
errMsg = multierror.Append(errMsg, fmt.Errorf("no service definition was found in the devfile sample"))
There was a problem hiding this comment.
oh should have used this
f1d0fd8 to
486b096
Compare
pkg/devfile/types.go
Outdated
There was a problem hiding this comment.
@jhadvig What's you opinion on this move? We have also helm, knative, terminal on this level, so it might be ok?
Alternatively, we could move all these packages into a devconsole package (another time)? Wdyt? 🤷♂️
christoph-jerolimov
left a comment
There was a problem hiding this comment.
Just found some nits, this should not block us from pushing this forward.
pkg/devfile/types.go
Outdated
There was a problem hiding this comment.
| // devfileData is the devfile-related information | |
| // DevfileData is the devfile-related information |
pkg/devfile/types.go
Outdated
There was a problem hiding this comment.
| // gitData is the git-related information | |
| // GitData is the git-related information |
pkg/devfile/resources.go
Outdated
There was a problem hiding this comment.
The console backend (this bridge code) doesn't clone the repo. So it might not be possible to access any resource from the FS.
I see that you checking the src.URL, src.Path or src.Data attributes in ReadKubernetesYaml [1], it might be good to have three more explicit API methods in the library. That would also remove the direct dependency to https://github.com/spf13/afero from our code.
Until then I would suggest:
| src := parser.YamlSrc{ | |
| Data: []byte(component.Kubernetes.Inlined), | |
| } | |
| fs := afero.Afero{Fs: afero.NewOsFs()} | |
| values, err := parser.ReadKubernetesYaml(src, fs) | |
| src := parser.YamlSrc{ | |
| Data: []byte(component.Kubernetes.Inlined), | |
| } | |
| fs := afero.Afero{Fs: afero.NewMemMapFs()} | |
| values, err := parser.ReadKubernetesYaml(src, fs) |
Or just using YAML.Unmarshal
There was a problem hiding this comment.
Good suggestion; I will put in a update to the library api method params.
but for now, I might go with NewMemMapFs(), dont really want to do YAML.Unmarshal because that would be just duplicating the devfile/library code in here to read the contents.
There was a problem hiding this comment.
Tested the following cases:
- Imported all devfile samples on 4.12 (Kubernetes resources are ignored)
- Basic Quarkus example doesn't start successfully, this issue is addressed here: devfile-samples/devfile-sample-code-with-quarkus#7
- Imported all devfile samples with this PR
- All samples could be imported successful 👍
- Basic Quarkus example could be imported from https://github.com/yangcao77/devfile-sample-code-with-quarkus
- Compared the
/api/devfile/response and it is identical to the old version [2] 👍
- Added the following code to
packages/dev-console/src/components/import/devfile/devfileHooks.ts[1] to insert someinlinedkubernetes resources from https://github.com/devfile-samples/devfile-sample-dotnet60-basic- The
/api/devfile/response contains an updatedDeploymentandServiceand these look fine to me 👍 ⚠️ Unfortunately, the import doesn't work because theRouteresource is broken. You can see the full JSON response below. [3]⚠️
- The
In the console the dry-run fail already:
The Deployment diff looks good to me:
The Service diff looks also fine:
Here you see the broken Route:
[1] code snippet for devfileHooks.ts to test inlined yaml
tsx
import { safeLoad, safeDump } from 'js-yaml';
/* eslint-disable no-console */
console.log('xxx oldDevfileContent', devfileContent);
const parsedDevfileContent = safeLoad(devfileContent);
console.log('xxx parsed devfileContent', devfileContent);
if (
parsedDevfileContent.metadata.name === 'dotnet60' &&
Array.isArray(parsedDevfileContent.components)
) {
console.log('xxx update devfile with metadata.name', parsedDevfileContent.metadata.name);
parsedDevfileContent.components.forEach((component) => {
if (component.kubernetes && component.kubernetes.uri === 'kubernetes/deployment.yaml') {
component.kubernetes = {
inlined: `kind: Deployment
apiVersion: apps/v1
metadata:
name: devfile-dotnet-deploy
spec:
replicas: 1
selector:
matchLabels:
app: devfile-dotnet-deploy
template:
metadata:
labels:
app: devfile-dotnet-deploy
spec:
containers:
- name: main
image: "{{CONTAINER_IMAGE}}"
imagePullPolicy: Always
resources: {}
`,
};
}
if (component.kubernetes && component.kubernetes.uri === 'kubernetes/service.yaml') {
component.kubernetes = {
inlined: `apiVersion: v1
kind: Service
metadata:
labels:
app: dotnet-deploy
name: devfile-dotnet-deploy
spec:
ports:
- name: http-8081
port: 8081
protocol: TCP
targetPort: 8081
selector:
app: devfile-dotnet-deploy
type: LoadBalancer
`,
};
}
});
} else {
console.log('xxx skip devfile with metadata.name', parsedDevfileContent.metadata.name);
}
const newDevfileContent = safeDump(parsedDevfileContent);
console.log('xxx newDevfileContent', newDevfileContent);
return {
name,
git: { URL: url, ref, dir: prefixDotSlash(dir) },
devfile: { devfileContent: newDevfileContent, devfilePath: `${smartSlashDir}${devfilePath}` },
};
}, [name, url, devfileContent, ref, dir, smartSlashDir, devfilePath]);[2] old and new json response from /api/devfile/ (when no inlined kubernetes resource is used)
{
"imageStream": {
"kind": "ImageStream",
"apiVersion": "image.openshift.io/v1",
"metadata": {
"creationTimestamp": null
},
"spec": {
"lookupPolicy": {
"local": false
}
},
"status": {
"dockerImageRepository": ""
}
},
"buildResource": {
"kind": "BuildConfig",
"apiVersion": "build.openshift.io/v1",
"metadata": {
"creationTimestamp": null
},
"spec": {
"source": {
"type": "Git",
"git": {
"uri": "https://github.com/devfile-samples/devfile-sample-dotnet60-basic.git"
},
"contextDir": "."
},
"strategy": {
"type": "Docker",
"dockerStrategy": {
"dockerfilePath": "docker/Dockerfile"
}
},
"output": {
"to": {
"kind": "ImageStreamTag",
"name": "dotnet60-basic:latest:latest"
}
},
"resources": {},
"postCommit": {},
"nodeSelector": null
},
"status": {
"lastVersion": 0
}
},
"deployResource": {
"kind": "Deployment",
"apiVersion": "apps/v1",
"metadata": {
"creationTimestamp": null
},
"spec": {
"selector": {
"matchLabels": {
"app": "dotnet60-basic"
}
},
"template": {
"metadata": {
"creationTimestamp": null
},
"spec": {
"containers": [
{
"name": "dotnet",
"image": "registry.access.redhat.com/ubi8/dotnet-60:6.0",
"command": [
"tail",
"-f",
"/dev/null"
],
"ports": [
{
"name": "http-8080",
"containerPort": 8080,
"protocol": "TCP"
}
],
"env": [
{
"name": "CONFIGURATION",
"value": "Debug"
},
{
"name": "STARTUP_PROJECT",
"value": "app.csproj"
},
{
"name": "ASPNETCORE_ENVIRONMENT",
"value": "Development"
},
{
"name": "PROJECTS_ROOT",
"value": "/projects"
},
{
"name": "PROJECT_SOURCE",
"value": "/projects"
}
],
"resources": {},
"imagePullPolicy": "Always"
}
]
}
},
"strategy": {
"type": "Recreate"
}
},
"status": {}
},
"service": {
"kind": "Service",
"apiVersion": "v1",
"metadata": {
"creationTimestamp": null
},
"spec": {
"ports": [
{
"name": "http-8080",
"port": 8080,
"targetPort": 8080
},
{
"name": "http-8081",
"port": 8081,
"targetPort": "8081"
}
]
},
"status": {
"loadBalancer": {}
}
},
"route": {
"kind": "Route",
"apiVersion": "route.openshift.io/v1",
"metadata": {
"creationTimestamp": null
},
"spec": {
"path": "/",
"to": {
"kind": "Service",
"name": "dotnet60-basic",
"weight": null
},
"port": {
"targetPort": "8081"
}
},
"status": {}
}
}[3] updated /api/devfile/ json response with inlined YAML
{
"imageStream": {
"kind": "ImageStream",
"apiVersion": "image.openshift.io/v1",
"metadata": {
"creationTimestamp": null
},
"spec": {
"lookupPolicy": {
"local": false
}
},
"status": {
"dockerImageRepository": ""
}
},
"buildResource": {
"kind": "BuildConfig",
"apiVersion": "build.openshift.io/v1",
"metadata": {
"creationTimestamp": null
},
"spec": {
"source": {
"type": "Git",
"git": {
"uri": "https://github.com/devfile-samples/devfile-sample-dotnet60-basic.git"
},
"contextDir": "."
},
"strategy": {
"type": "Docker",
"dockerStrategy": {
"dockerfilePath": "docker/Dockerfile"
}
},
"output": {
"to": {
"kind": "ImageStreamTag",
"name": "dotnet60-basic:latest:latest"
}
},
"resources": {},
"postCommit": {},
"nodeSelector": null
},
"status": {
"lastVersion": 0
}
},
"deployResource": {
"kind": "Deployment",
"apiVersion": "apps/v1",
"metadata": {
"name": "devfile-dotnet-deploy",
"creationTimestamp": null
},
"spec": {
"replicas": 1,
"selector": {
"matchLabels": {
"app": "devfile-dotnet-deploy"
}
},
"template": {
"metadata": {
"creationTimestamp": null,
"labels": {
"app": "devfile-dotnet-deploy"
}
},
"spec": {
"containers": [
{
"name": "main",
"image": "{{CONTAINER_IMAGE}}",
"resources": {},
"imagePullPolicy": "Always"
}
]
}
},
"strategy": {}
},
"status": {}
},
"service": {
"kind": "Service",
"apiVersion": "v1",
"metadata": {
"name": "devfile-dotnet-deploy",
"creationTimestamp": null,
"labels": {
"app": "dotnet-deploy"
}
},
"spec": {
"ports": [
{
"name": "http-8081",
"protocol": "TCP",
"port": 8081,
"targetPort": 8081
}
],
"selector": {
"app": "devfile-dotnet-deploy"
},
"type": "LoadBalancer"
},
"status": {
"loadBalancer": {}
}
},
"route": {
"metadata": {
"creationTimestamp": null
},
"spec": {
"to": {
"kind": "",
"name": "",
"weight": null
}
},
"status": {}
}
}|
@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 |
@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. |
|
@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. Screen.Recording.2022-10-17.at.4.41.20.PM.mov |
Signed-off-by: Maysun J Faisal <[email protected]>
Signed-off-by: Maysun J Faisal <[email protected]>
Signed-off-by: Maysun J Faisal <[email protected]>
Signed-off-by: Maysun J Faisal <[email protected]>
Signed-off-by: Maysun J Faisal <[email protected]>
Signed-off-by: Maysun J Faisal <[email protected]>
Signed-off-by: Maysun J Faisal <[email protected]>
Signed-off-by: Maysun J Faisal <[email protected]>
486b096 to
7952114
Compare
|
@divyanshiGupta @jerolimov I have rebased and addressed the PR concerns as well as our internal Slack thread conversation. PTAL. I have tested 3 cases:
|
pkg/devfile/types.go
Outdated
| Service *corev1.Service `json:"service"` | ||
| Route *routev1.Route `json:"route"` |
There was a problem hiding this comment.
This pointers return this json if the Service or route is not defined:
{
/* ... */
"service": null,
"route": null
}You can omit this null values, this would fix the crash you mentioned here #12000 (comment). But it still need some frontend changes of PR #12168. So its up to you if you want apply this small change or not:
| Service *corev1.Service `json:"service"` | |
| Route *routev1.Route `json:"route"` | |
| Service *corev1.Service `json:"service,omitempty"` | |
| Route *routev1.Route `json:"route,omitempty"` |
There was a problem hiding this comment.
@jerolimov oops, I have committed this. if you can, please lgtm it again, thanks!
christoph-jerolimov
left a comment
There was a problem hiding this comment.
Overall this change looks good to me. 👍
I have retested this PR alone and together with #12168:
- Imported all devfile samples without inlined frontend change
- 5/6 samples could be imported successfully:+1:
- The Quarkus example doesn't work.
⚠️ It works with this PR, please take a look into this PR: devfile-samples/devfile-sample-code-with-quarkus#7 - Compared the
/api/devfile/response and it is 100% identical to the old version 👍
- With the code snippet from #12000 (review) and also together with the PR #12168:
- tested with an inlined Service and Deployment: both are created, Route is null and isn't created 👍
- tested with just a Deployment, Service and Route are null and not created 👍
I observed one thing we should check on Slack. The Deployment contains a "variable" "{{CONTAINER_IMAGE}}" as image container. The frontend automatically overrides this with the new (ImageStream) name plus :latest. I expect this is fine for phase one, but that we should improve this behaviour later.
createOrUpdateDeployment is called here
and then overrides the image in line 368 here:
The Deployment JSON as reference:
"deployResource": {
"kind": "Deployment",
"apiVersion": "apps/v1",
"metadata": { "name": "devfile-dotnet-deploy", "creationTimestamp": null },
"spec": {
"replicas": 1,
"selector": { "matchLabels": { "app": "devfile-dotnet-deploy" } },
"template": {
"metadata": {
"creationTimestamp": null,
"labels": { "app": "devfile-dotnet-deploy" }
},
"spec": {
"containers": [
{
"name": "main",
"image": "{{CONTAINER_IMAGE}}",
"resources": {},
"imagePullPolicy": "Always"
}
]
}
},
"strategy": {}
},
"status": {}
},/lgtm
/approve
@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. |
|
/label docs-approved |
Co-authored-by: Christoph Jerolimov <[email protected]>
|
/lgtm |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Verified on cluster created on pr |
|
/retest |
|
@maysunfaisal: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
Description
This PR updates the Devfile handler backend code to consume the Kubernetes YAML file definitions. For now, the devfile samples only have the
Deploymentdefinition in the YAML file but the next course of action would be to update theService,Routedefinitions in the devfile sampleFixes devfile/api#924
This PR just added the ability to read from Kubernetes YAML if the Devfile Kubernetes Component has
inlineddefinition. 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 branchThis 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 %:
How to test
(Check history for prev edit)