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

update to pull samples from devfile registry #10394

Merged

Conversation

yangcao77
Copy link
Contributor

@yangcao77 yangcao77 commented Nov 3, 2021

Signed-off-by: Stephanie [email protected]

Fixes issue: devfile/api#590

This PR removes the hardcoded sample and pull samples from devfile regstry (https://registry.devfile.io)
The PR updates devfile handler to support devfile 2.2.0

@openshift-ci openshift-ci bot added the component/backend Related to backend label Nov 3, 2021
@openshift-ci openshift-ci bot added component/dev-console Related to dev-console kind/dependency-change Categorizes issue or PR as related to changing dependencies labels Nov 3, 2021
Signed-off-by: Stephanie <[email protected]>
@yangcao77
Copy link
Contributor Author

/test e2e-gcp-console

@yangcao77
Copy link
Contributor Author

@rohitkrai03 @maysunfaisal
I have no permissions to add reviewers. Can you help review this PR?

@@ -26,20 +26,20 @@ func TestGetRegistrySamples(t *testing.T) {
}{
{
name: "Fetch the sample placeholder",
registry: "sample-placeholder",
registry: DEVFILE_REGISTRY_URL,
Copy link
Member

Choose a reason for hiding this comment

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

we dont want to ping the actual registry for tests and spam the telemetry. https://registry.stage.devfile.io would be preferred here

@yangcao77 yangcao77 changed the title update to pull samples from devfile registry <WIP>update to pull samples from devfile registry Nov 4, 2021
@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 4, 2021
Signed-off-by: Stephanie <[email protected]>
Signed-off-by: Stephanie <[email protected]>
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 10, 2021
@yangcao77 yangcao77 changed the title <WIP>update to pull samples from devfile registry update to pull samples from devfile registry Nov 10, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 10, 2021
@yangcao77
Copy link
Contributor Author

/test e2e-gcp-console

Copy link
Member

@maysunfaisal maysunfaisal left a comment

Choose a reason for hiding this comment

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

reviewed the Go aspects of this PR

const DEVFILE_REGISTRY_PLACEHOLDER_URL = "sample-placeholder"
const DEVFILE_REGISTRY_URL = "https://registry.devfile.io"
const DEVFILE_STAGING_REGISTRY_URL = "https://registry.stage.devfile.io"
const ODC_TELEMETRY_CLIENT_NAME = "odcsample"
Copy link
Member

Choose a reason for hiding this comment

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

I would perhaps name it openshift-console.. Check with Kim I guess what the other teams are doing?

Copy link
Contributor Author

@yangcao77 yangcao77 Nov 16, 2021

Choose a reason for hiding this comment

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

odcsample was suggested by Kim. I confirmed with her, and she is fine switching to use openshift-console. Updated


pythonBase64Image := "iVBORw0KGgoAAAANSUhEUgAAAEIAAABCCAYAAADjVADoAAAABGdBTUEAALGPC/xhBQAAACBjSFJNAAB6JgAAgIQAAPoAAACA6AAAdTAAAOpgAAA6mAAAF3CculE8AAAAaGVYSWZNTQAqAAAACAAEAQYAAwAAAAEAAgAAARIAAwAAAAEAAQAAASgAAwAAAAEAAgAAh2kABAAAAAEAAAA+AAAAAAADoAEAAwAAAAEAAQAAoAIABAAAAAEAAABCoAMABAAAAAEAAABCAAAAAGpSBCwAAALiaVRYdFhNTDpjb20uYWRvYmUueG1wAAAAAAA8eDp4bXBtZXRhIHhtbG5zOng9ImFkb2JlOm5zOm1ldGEvIiB4OnhtcHRrPSJYTVAgQ29yZSA2LjAuMCI+CiAgIDxyZGY6UkRGIHhtbG5zOnJkZj0iaHR0cDovL3d3dy53My5vcmcvMTk5OS8wMi8yMi1yZGYtc3ludGF4LW5zIyI+CiAgICAgIDxyZGY6RGVzY3JpcHRpb24gcmRmOmFib3V0PSIiCiAgICAgICAgICAgIHhtbG5zOnRpZmY9Imh0dHA6Ly9ucy5hZG9iZS5jb20vdGlmZi8xLjAvIgogICAgICAgICAgICB4bWxuczpleGlmPSJodHRwOi8vbnMuYWRvYmUuY29tL2V4aWYvMS4wLyI+CiAgICAgICAgIDx0aWZmOkNvbXByZXNzaW9uPjE8L3RpZmY6Q29tcHJlc3Npb24+CiAgICAgICAgIDx0aWZmOlJlc29sdXRpb25Vbml0PjI8L3RpZmY6UmVzb2x1dGlvblVuaXQ+CiAgICAgICAgIDx0aWZmOk9yaWVudGF0aW9uPjE8L3RpZmY6T3JpZW50YXRpb24+CiAgICAgICAgIDx0aWZmOlBob3RvbWV0cmljSW50ZXJwcmV0YXRpb24+MjwvdGlmZjpQaG90b21ldHJpY0ludGVycHJldGF0aW9uPgogICAgICAgICA8ZXhpZjpQaXhlbFhEaW1lbnNpb24+NjY8L2V4aWY6UGl4ZWxYRGltZW5zaW9uPgogICAgICAgICA8ZXhpZjpDb2xvclNwYWNlPjE8L2V4aWY6Q29sb3JTcGFjZT4KICAgICAgICAgPGV4aWY6UGl4ZWxZRGltZW5zaW9uPjY2PC9leGlmOlBpeGVsWURpbWVuc2lvbj4KICAgICAgPC9yZGY6RGVzY3JpcHRpb24+CiAgIDwvcmRmOlJERj4KPC94OnhtcG1ldGE+CiQPAV8AABBVSURBVHgB7Vt9dBXFFZ/dffkGEmISIYSEYAhKjKlgRTBaUFoFi1IFxY/SD49Qi/X0HE/VeqSntp5WPVVr29NWa1EUPVLrQUSRYkQPICCoYCBBvgnhKyEkIeS95L23b7f3zuzdN293X/KQpad/MMlm5s7cuXN/v7mzO/sRxs6lcwzIDCiycFbKhVUDBg9Uy4vHXT8sp6BkhBkJDzMVs0AxzUGmyTJMxjTTNCLMNENwdBpMaVVVrbn58/ebwu1tR7sONew5K345jJ4lIoqzL5n9g2l5wy+cmTd8dI1h6OUMQQNyAA0HwMeygbJ1MKgHmdfbdawz0t25I9iy/7Wt773yCmvbecrhv2+i70TU3P5QbcnY655LyxowNhaNMD3SC85KoLGMRBgA3CpzMpAEkpEg+AFmoE5halomi4S6t7bWf/TTPSuf3+AbesmQJpXPuFhzy8OXVky5c1UsGi5HAnrDERbRYyyqQ8AD5SrSjiTgjHPQWBQylHiZk8JJQBnbDYaEKqoyJG/ExTPT84eub9+xoemMnXUY8JMI5bK7f/N3IAGWAoKPsZqywWz62OGsrCCHNbcFWW9E54RwsBZIRCtkkVvo7Touc5KAkHBv5sDiUVd3HNv9euTE0ZADyxmJvhFxXknNsPJJMx/Xw7054WiMXX3R+eyxmTWspjSfja8oZGWFA9jar46yGCwJnqxIcJLglkXUYD1GDSyV/PSM7BNtDes+EYb8+av6Y4YxPT02NGbECtBVXAK3XF7KAmrc/MTKIjZmWB4LR3VEZs+4wMfDw67jUZCgE2839DDLOX/kTTiMX76jHd+MqYqZAQDgqihOgppEAjmswYkCgcdnHVqSACYdq4Pdx4jFWCA7t4jlVwwgu37kvhHBneEoITrg5PjeF80J/m1v7mCNhzpYuoZkiBkWJMQvp0KOR0tcFuShjAeQobBYxLdljY4GErw9EyEadzYjoLIVWw6y7t4ImzCqiB3tCLJlnx1gwXCUBWDZ8NmWrhI4LMC361FCwLYeyUILLqjQ7nPyjwgEg85bP3ieqNt2mK36spnXp0EF8BPXsXQRD+9HsgcBoCCsWm0o+538IyIAc+1wFCMDThtwwJXCaiPQKMt1Thn15HYhUp115fGRDd+IUAxTV9IzQ6oRAwwCOAKBs6cDEMjWztEwDcOMRLJg0QegFtTFckhGChGj0CX4/5GI9gP1G+t+d9sYRVHFdOHOOhMOzBNSjyQpZuWN9/110LDR041omBNBYIkUr8gQRBmw+PxLpxURA4srC2DfX6pqSjZcGnAS7aQrsIuIwF6YajOgCcuYJ6QsS4KzK/xCdIAGKHpFjqsO9Hm0QR7I1CsqBhQOHZo3hkUN2JyIpEEQ6ooaDcTCTR9vPn6M6vvLUyKi+taHrs4fUXVvXunoCaYeGw4XPFz88ItH/PKXcDcp1zv17L4m3EeE4UrYSzxYNgUxzqhAJazjy619T+ilhd+bUXtj9SIWjLhxqsqhbduOrXhgQd1jH3zQfMStkFjTNxEVFRmTZ/36qbySyvm6HtWioW5vR3FG8W6SAIJs31JbZTpvQAtvQ12MBEEeBgRviUcGtuEP6lltcfschBLuhd0VkhDE0HIk0yypHlM4d/nim781Y/abN6386MhOh0aC2NeGKjB9/sKXcoeNuh9ugTW44eGOofO2Y8LV+C01yQQSZbqlhmEJGOZcIoBSP66DskWA1YvLol+C/94CLh8WY7CRYRkqG7309RmLJk0agWespCkpEZMefu0BNS3j9mhP0HLTIoCcJ0dtGfmRdLDeaoOCKFvtskx94nXedrCdH0mhYAPqAAH8PIJXLijDJi4zJ238b++vubuvrp5EDB5ZWzq4ZNSDOicB7bsBxh1HkMl1ZD1RRnOJxDhluQ8RxfO+kHDwFgkYDUSGFRlV1fn3VFS4T91k0pOIktqpt8M1Ph/xOZ1HOZmjTkAkx/tg1/jJtT878X7cE/IZ7sRhl4YJnveARfjFpYARIB9ABhGi62xwQXr1gnkTx2I3r+RFhDK47KIb8Ex+Oo4KgDCEgyinDXmG3URRpAg7drvDc0XR8OyowwV7kACPEUBRQLlMCpRjulpemjPFYcoWXUTcfNfcIXCbW2nEdMAgHHOC8wKD+GXQyfoIHVSN26Yy2U2wY7sqCrBhY+HuroM4WkFhThXQASUCDWUeGVhH9VgHR0+UXVyTXyusuP+6iBg+YfoFphErEhMrLwPhfN8AvcERQAEY7PBwlmxjjRiQtwk9ZNad1PQMdrxp7xvQooydOPwq1hOGIoEmQiC3CbHq+PLIqKytzh3sturxYCarsGwMeGWtPeEgOo4/fKasMq8RYSBqcIZRBxK1Cdndz36CLTS5XasXTG7cjtNhjIZYOFTfUvfHRS88ceX1EO5VfUcDEiKRFI0WRwJKqdMuyq6IMPTIaJoR4ZIAYpOAgK3Z485bMgdNZcyRFJJRgjLKvI7LWJR0JF2u5/AWSdCyBh5rXFv3/aoqpt1xZ82TrCcCO1wJaH9lZqbfe9tITyI8dpZmqU2Ey1Fv522AFhhbhtnFx9ZqIB2qMETh6RIcuEVOHANO/1KdgwMwocJ7jWBd4ydvz49sf2vX5sPzFmelsWoWBhLsqwWMRWU+CTgeHFTmbSYbNTKnzGkfZRcR4GAeAcE80WH3DLp1cHAVwAeYqSgHI8HO/d37t50ES3jdhLt1dBjdAz0kCn6xBy0XLIqk4KuQcChi7u1pP76iY9PitXdOzR/04vJ7lmSmabfiRskLqKgDi3wczK0DB4IUCetFwn7iXxcR0JwGTicQgGQkAgYtR52QDf5WCnaja5rWr3z6yJqVdYwdOeP3D7OmDil8pO6W+d/4ZvEv4N6iDF6QoANwOIDyCLBI8IgGzrhpej70dREB+KJ9RwH6IN1g2YQYTMsYoB+rX/PozqVPPQla7PlnJ99wQdn4azQzrRioFZsgbOgr8YmLwXIALwwzfcK4oqLMgqwL4ZVZAesSrw9tApAMF3giRyKK9NBXEX8uDzyIMDtp9iknYryjQEQPXNaM5i2r5+1755mFLzw7acqc2Zc+k5GbVQ1vdFyDJq3gJzsHgB6Y/ZP4MEcGbc06RQS1JUSBpS8TBXrw4DDoNb6LCGC42V4K0JH/0KyjBGXezl0TZTWQxjqb9/4ZSVj68rRbZ9w19lXW0ZvOuvAan0JKcDaFGZUBp9wX7arwOsHwfDbhIkIJBHZysBZoHgUSIUgMJiIEc0UNHK7/cNFjd91YPhpIeJG19aSnAB+twC9GDM0e2raI4Dm1Y53UJpftvpKdpH1NtutAaI+Xby4igscON3LbOBgcMmDujF0nnMNoaD+48012cFvHLxfP+RPrjg70GshVJxOQ1HEHAQT6dPVxLMSjKCf/tmS/JxGuDdXhTe/shTlu4U+abdDepKBt3Ce07tz+7viK/EFjLhs2hfXqLsyJFWgLZw+dc2yGeJ2jXt4ZOvU5QIe+lw7ahWfKHSfCjZvqe/E+xZVcRLzzxsKj4e6OXfgmF6MhMQpkGZsgYuAxfkvDh01anlnKYuYQ1wgJFRYJsrNeQL82SbTBchCMhMEj4u1bO98Hd1DJlVxEgIbZdXDnKgV2g0iCc2l4yHCt02KqobmeVyeMxmfPw0GqTykakgC1+2J0WAcn2BoPvtdgSiyy8O3WtxN8kgQvItjR+rqluKsj0M6owHpBEg4KZdgj6BBAkl2piLoOAmzHHfUEwqlv1ycByvXBliuSQB/rVJMFu2JrXl7W0iA5llD0JKJ7z+aGSE9wCa5/Dhgw2qSIM6klY3MS/DgMzTbmX2sJWETJs5tAkkSMPBYRQwTmaOxfy1qfAyfQEc/kSQRq1r/19K8CmTl8T8FJ4BPOzwlIS/zHkwiMAhzTw1GbEAQpAyVdqnPICWQ6+1p9iAwaF31Ogwc5wdi7P16we4UnA1ZlUiI6d6xvOrxl9Wy49T2kammAyYoKpMBaGlBwRwTNQsKsyI4SQMqpDXJPklCPdKwyAZXr7b6WXQSYrcENqrF91tyG+SBBQ/Lk2kfIql/9+8n1xxo214688ruP5Awtn2bGjBJ7iQBgvJNUAxnwpQzQjonPGjriiAicGSKIyhwM1MP6ZRl4GwImoChsOMvYHxtxqaIuypij7CjTIxZVaalv6H73wSd2LPjPutBRUO4z9UkE9uzcsbrpix2r5+UUVg3R0o0RhmImvihRFIN17T2coRUV2mBdhKCzOCHWbHFQUM5U2OZPWx//ycMb3+vTS0cjTIYyIMtM1zT87ART4t4FLmNRRdEPrtnck/jZjlD2/NsvEdQreLwBX6ji4ZkMFT+GwBDGA2fPAk0RwImQCaGZNNv0iLEvlgk3AimkTMNUQiwU2bKFnUhBPWUVi9GU9ZMqXnVF8ZVr3p+1Dr4PsmbfCZoiQiIJCVONELyXg28C8DW/HDFWf04gnSOwL6yJHLZv8nc+v+bjrZ2dSR06zYaUIyI1u5bDCYD6iYyYmc2CerYgwUES2eG5RQwvq/l6LMXnG6k57n5Ul2I/bzWcPVwatBxcAGSgGCEyOCpbuQkPYfCzBx4RECwKXLmIGNNxUvD25rRqfYwIdBpJoPDuB6iLJEnfABIyKxnLvQ7sATGdsAUIwaZQSfHu/rQoEMo+EoE+SyTwmaRZpnqQiSiKhgQ9aDfgHJMxkrHSP8B/cuQKL3O/zdh+2Ar07j5rZKR0phbe9P1XiQEyTgSeJygyrDIRRCTYehgFSBLqWTkSMejaOAk4rJYnooPbBTmgmtHskziIb8k3IuC+5Cjc4XXFo8IC5wRqkyQTgIQQGVjv9QWMhRu+3O1oix7/9FPW7RsLYMg3ItZsamkKdfV+jA9A7PC3Z9oJ1IskJADq8UYPHxtEW+M4oy3wABfqFFjJsG1uaOh6SwwSVznTkp/fM5vlRemNY2uL7mChKOw+pfNBfxssXBZ0zsC50dvgs5+NECRd8H3UFsZa/gLnh/1wAs1k8N3U1vse3f+zPYf5298zxW/3921DRRaXvnDF1BkzR7zKOnvPY/Bxuh0dNlCceSCJg7fKdO4gI5jj8jBp6wyRMBBI6DG23Tz3y5kr1p3aJav6UfYzIrg/S5Yf2rNv14nl064dMjwwUBvN4FaEwT+yCEKkKCFiKBqcaBRwDfcOmfDgKyc9uK0x+Pycnzf+aNWGU4ecqn7IvkeE7NQ/fn/xpMtrcn94SU3uJPigo4yTAV8Esh4iBiNGSnAixM/gRIKw0dTG+vpTK5568cDLry1rb5Q0fS+eVSLI23GVrGDOzGE15WXZV5yXrVw2cdygElg2+dAOn+Hix6uwU9TUUydOhNs31HfvZhrb+M83Wzd/tjvUeOgQw9dcZz39T4jwQmF2rh3MdC0brgQqM1T4TunUKaVosq+XRK9xz9WdYyA1Bs7m0pBty+VUPMPLCyW5THW+56frIDlA/TDHA0/1XmWqk3OnDRkolpMdeImhNrmM9mQbKJ92QgdTSQQEASc7cE/i1YZ9ZaLIljwuAaScgGLuPKxrr6ue9MiGbL/fcn9EOIERWMxTObA/9SEyMMdEY9NsOsETYMxTObA/9SFSME8p/ReuClWyHxZNZAAAAABJRU5ErkJggg=="
pythonBase64Image := "https://www.python.org/static/community_logos/python-logo-generic.svg"
Copy link
Member

Choose a reason for hiding this comment

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

I havent tried it out locally yet on an OpenShift cluster but does the image render if its a direct URL? IIRC ODC wanted icons to be in base64

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tested, the icons worked. I updated the samples to use url for svg as I saw other ODC frontend for other icons are using url for svg file, and also their test for samples are also using that.

if len(containerComponents) != 1 {
errMsg := "Console Devfile Import Dev Preview, supports only one component container with attribute 'tool: console-import'"
if len(imageComponents) != 1 {
errMsg := fmt.Sprintf("Console Devfile Import Dev Preview, supports only one image component with attribute 'tool: console-import', now only has %v", len(imageComponents))
Copy link
Member

Choose a reason for hiding this comment

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

but we are not filtering tool: console-import, so maybe update the err msg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

return
}

dockerRelativeSrcContext := imageComponents[0].Image.Dockerfile.BuildContext
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

should be if dockerRelativeSrcContext == ""?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch. updated

RouteSpecParams: generator.RouteSpecParams{
ServiceName: data.Name,
PortNumber: intstr.FromString(imagePort),
Path: "/",
Copy link
Member

Choose a reason for hiding this comment

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

this is just a mere FYI, earlier when this was first written, we were thinking about the possibility of having 1+ routes for a sample and if that would be the case I wonder how would one feed devfile these info since image component has no endpoint arr, but thats just a thought, this is fine for now.

Are we saying all samples will have a path /, wondering if this should also be in the devfile attributes along with the port 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking the same thing, if ODC is going to support 1+ images or 1+ routes. but that needs more discussion.
since the PR is only for switching to use 2.2.0 devfile samples from registry, I think the current change is fine.
we can always submit new PRs once we decided to support more in ODC.

Copy link
Contributor Author

@yangcao77 yangcao77 Nov 16, 2021

Choose a reason for hiding this comment

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

In addition, we should an issue to encourage odc to switch to consume the deploy commands and Kubernetes components, after this PR got merged. if that's the case, the routes and path will no longer be a problem.

Signed-off-by: Stephanie <[email protected]>
Copy link
Contributor

@rottencandy rottencandy left a comment

Choose a reason for hiding this comment

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

Fronted-related changes look good to me.
cc: @invincibleJai

@invincibleJai
Copy link
Member

/lgtm

frontend related changes look good, will need help with Jakub/Bryan for go code review.

@sanketpathak can do QE review pls

cc @jhadvig / @florkbr

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 19, 2021
@sanketpathak
Copy link
Contributor

Tested the pr with cluster-bot, it is pulling samples from devfile regstry (https://registry.devfile.io)


/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Nov 19, 2021
@yangcao77
Copy link
Contributor Author

/test e2e-gcp-console

Copy link
Contributor

@rottencandy rottencandy left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 30, 2021
@yangcao77
Copy link
Contributor Author

@christianvogt Can you add an /approve label to this PR? Thanks

@rottencandy
Copy link
Contributor

/cc @spadgett

@openshift-ci openshift-ci bot requested a review from spadgett December 2, 2021 15:11
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

/approve

vendor/OWNERS Outdated
@@ -1,8 +0,0 @@
reviewers:
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to remove this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't mean to remove it. Adding that file back. Thanks for catching it.

@@ -11,7 +11,7 @@ const normalizeDevfile = (devfileSamples: DevfileSample[], t: TFunction): Catalo
const normalizedDevfileSamples = devfileSamples?.map((sample) => {
const { name: uid, displayName, description, tags, git, icon } = sample;
const gitRepoUrl = Object.values(git.remotes)[0];
const iconUrl = icon ? `data:image/png;base64,${icon}` : '';
const iconUrl = icon || '';
Copy link
Member

Choose a reason for hiding this comment

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

Do we know if the icons will always be URLs instead of base64 encoded images?

Can we use icon directly and remove the iconUrl var now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The icons from registry will always be URLs. I have removed iconUrl and use icon directly

@spadgett
Copy link
Member

spadgett commented Dec 2, 2021

/hold

I had a few small comments, but adding the approved label.

Signed-off-by: Stephanie <[email protected]>
@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm Indicates that a PR is ready to be merged. labels Dec 2, 2021
@yangcao77
Copy link
Contributor Author

@spadgett I have pushed a new commit to address the comments you had. Can you help take a quick look again to see if you have more concerns?
If not, can you help add back a lgtm label? Thanks

@spadgett
Copy link
Member

spadgett commented Dec 3, 2021

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 3, 2021
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 3, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: invincibleJai, rottencandy, spadgett, yangcao77

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 openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2021
@openshift-merge-robot openshift-merge-robot merged commit d31fcfb into openshift:master Dec 3, 2021
@rottencandy
Copy link
Contributor

Backporting the changes to 4.9 to fix https://bugzilla.redhat.com/show_bug.cgi?id=2025592
The current devfile handler for 4.9 does not work for devfile v2.2
According to the discussion on slack: https://coreos.slack.com/archives/C017YDKHQV6/p1637746963035100

/cherrypick release-4.9

@openshift-cherrypick-robot

@rottencandy: #10394 failed to apply on top of branch "release-4.9":

Applying: update to pull samples from devfile registry
Using index info to reconstruct a base tree...
M	go.mod
M	go.sum
M	vendor/golang.org/x/sys/unix/mkerrors.sh
M	vendor/golang.org/x/sys/unix/syscall_darwin.go
M	vendor/golang.org/x/sys/unix/syscall_linux.go
A	vendor/golang.org/x/sys/unix/syscall_linux_ppc.go
M	vendor/golang.org/x/sys/unix/syscall_linux_s390x.go
M	vendor/golang.org/x/sys/unix/zerrors_darwin_amd64.go
M	vendor/golang.org/x/sys/unix/zerrors_darwin_arm64.go
M	vendor/golang.org/x/sys/unix/zerrors_freebsd_arm.go
M	vendor/golang.org/x/sys/unix/zerrors_linux.go
M	vendor/golang.org/x/sys/unix/ztypes_darwin_amd64.go
M	vendor/golang.org/x/sys/unix/ztypes_darwin_arm64.go
M	vendor/golang.org/x/sys/unix/ztypes_linux.go
A	vendor/golang.org/x/sys/unix/ztypes_linux_ppc.go
M	vendor/golang.org/x/sys/windows/types_windows.go
M	vendor/k8s.io/apimachinery/pkg/util/httpstream/httpstream.go
M	vendor/k8s.io/apimachinery/pkg/util/httpstream/spdy/connection.go
M	vendor/modules.txt
M	vendor/sigs.k8s.io/controller-runtime/pkg/client/apiutil/apimachinery.go
M	vendor/sigs.k8s.io/controller-runtime/pkg/client/client.go
M	vendor/sigs.k8s.io/controller-runtime/pkg/client/client_cache.go
M	vendor/sigs.k8s.io/controller-runtime/pkg/client/interfaces.go
M	vendor/sigs.k8s.io/controller-runtime/pkg/client/metadata_client.go
M	vendor/sigs.k8s.io/controller-runtime/pkg/client/namespaced_client.go
A	vendor/sigs.k8s.io/controller-runtime/pkg/log/deleg.go
A	vendor/sigs.k8s.io/controller-runtime/pkg/log/null.go
A	vendor/sigs.k8s.io/controller-runtime/pkg/log/warning_handler.go
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): vendor/sigs.k8s.io/controller-runtime/pkg/log/warning_handler.go deleted in HEAD and modified in update to pull samples from devfile registry. Version update to pull samples from devfile registry of vendor/sigs.k8s.io/controller-runtime/pkg/log/warning_handler.go left in tree.
CONFLICT (modify/delete): vendor/sigs.k8s.io/controller-runtime/pkg/log/null.go deleted in HEAD and modified in update to pull samples from devfile registry. Version update to pull samples from devfile registry of vendor/sigs.k8s.io/controller-runtime/pkg/log/null.go left in tree.
CONFLICT (modify/delete): vendor/sigs.k8s.io/controller-runtime/pkg/log/deleg.go deleted in HEAD and modified in update to pull samples from devfile registry. Version update to pull samples from devfile registry of vendor/sigs.k8s.io/controller-runtime/pkg/log/deleg.go left in tree.
Auto-merging vendor/sigs.k8s.io/controller-runtime/pkg/client/namespaced_client.go
Auto-merging vendor/sigs.k8s.io/controller-runtime/pkg/client/metadata_client.go
Auto-merging vendor/sigs.k8s.io/controller-runtime/pkg/client/interfaces.go
Auto-merging vendor/sigs.k8s.io/controller-runtime/pkg/client/client_cache.go
Auto-merging vendor/sigs.k8s.io/controller-runtime/pkg/client/client.go
CONFLICT (content): Merge conflict in vendor/sigs.k8s.io/controller-runtime/pkg/client/client.go
Auto-merging vendor/sigs.k8s.io/controller-runtime/pkg/client/apiutil/apimachinery.go
CONFLICT (content): Merge conflict in vendor/sigs.k8s.io/controller-runtime/pkg/client/apiutil/apimachinery.go
Auto-merging vendor/modules.txt
CONFLICT (content): Merge conflict in vendor/modules.txt
Auto-merging vendor/golang.org/x/sys/windows/types_windows.go
CONFLICT (modify/delete): vendor/golang.org/x/sys/unix/ztypes_linux_ppc.go deleted in HEAD and modified in update to pull samples from devfile registry. Version update to pull samples from devfile registry of vendor/golang.org/x/sys/unix/ztypes_linux_ppc.go left in tree.
Auto-merging vendor/golang.org/x/sys/unix/ztypes_linux.go
CONFLICT (content): Merge conflict in vendor/golang.org/x/sys/unix/ztypes_linux.go
Auto-merging vendor/golang.org/x/sys/unix/ztypes_darwin_arm64.go
Auto-merging vendor/golang.org/x/sys/unix/ztypes_darwin_amd64.go
Auto-merging vendor/golang.org/x/sys/unix/zerrors_linux.go
Auto-merging vendor/golang.org/x/sys/unix/zerrors_freebsd_arm.go
Auto-merging vendor/golang.org/x/sys/unix/zerrors_darwin_arm64.go
Auto-merging vendor/golang.org/x/sys/unix/zerrors_darwin_amd64.go
Auto-merging vendor/golang.org/x/sys/unix/syscall_linux_s390x.go
CONFLICT (modify/delete): vendor/golang.org/x/sys/unix/syscall_linux_ppc.go deleted in HEAD and modified in update to pull samples from devfile registry. Version update to pull samples from devfile registry of vendor/golang.org/x/sys/unix/syscall_linux_ppc.go left in tree.
Auto-merging vendor/golang.org/x/sys/unix/syscall_linux.go
Auto-merging vendor/golang.org/x/sys/unix/syscall_darwin.go
Auto-merging vendor/golang.org/x/sys/unix/mkerrors.sh
CONFLICT (content): Merge conflict in vendor/golang.org/x/sys/unix/mkerrors.sh
Removing vendor/github.com/hashicorp/go-multierror/.travis.yml
Removing vendor/github.com/ghodss/yaml/yaml_go110.go
Removing vendor/github.com/ghodss/yaml/yaml.go
Removing vendor/github.com/ghodss/yaml/go.sum
Removing vendor/github.com/ghodss/yaml/go.mod
Removing vendor/github.com/ghodss/yaml/fields.go
Removing vendor/github.com/ghodss/yaml/README.md
Removing vendor/github.com/ghodss/yaml/LICENSE
Removing vendor/github.com/ghodss/yaml/.travis.yml
Removing vendor/github.com/ghodss/yaml/.gitignore
Removing vendor/github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2/WorkspacePodContribution.go
Removing vendor/OWNERS
Auto-merging go.sum
CONFLICT (content): Merge conflict in go.sum
Auto-merging go.mod
CONFLICT (content): Merge conflict in go.mod
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 update to pull samples from devfile registry
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

Backporting the changes to 4.9 to fix https://bugzilla.redhat.com/show_bug.cgi?id=2025592
The current devfile handler for 4.9 does not work for devfile v2.2
According to the discussion on slack: https://coreos.slack.com/archives/C017YDKHQV6/p1637746963035100

/cherrypick release-4.9

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/backend Related to backend component/dev-console Related to dev-console docs-approved Signifies that Docs has signed off on this PR kind/dependency-change Categorizes issue or PR as related to changing dependencies lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants