fix #540 #246

Horiodino · 2024-06-11T15:13:02Z

Please specify the area for this PR

What does does this PR do / why we need it:

Which issue(s) this PR fixes:

Fixes #devfile/api#540

PR acceptance criteria:

Test Coverage
- Are your changes sufficiently tested, and are any applicable test cases added or updated to cover your changes?

Documentation (WIP)

Does the REST API doc need to be updated with your changes?
Does the registry library doc need to be updated with your changes?

How to test changes / Special notes to the reviewer:

Signed-off-by: Horiodino <holiodin@gmail.com>

Jdubrick · 2024-06-11T15:18:40Z

/ok-to-test

Jdubrick · 2024-06-11T15:46:56Z

All the checks pass in our CI - Are you still experiencing issues building locally @Horiodino

fyi @michael-valdron

Horiodino · 2024-06-11T16:05:18Z

is the code working as expected ?

Horiodino · 2024-06-11T16:13:14Z

yep i can build image and test case is running!

Jdubrick · 2024-06-11T16:14:23Z

yep i can build image and test case is running!

No issues locally anymore? @Horiodino

Horiodino · 2024-06-11T16:17:02Z

nope

Jdubrick · 2024-06-11T16:40:29Z

index/server/pkg/server/endpoint.go

@@ -543,6 +543,12 @@ func buildIndexAPIResponse(c *gin.Context, indexType string, wantV1Index bool, p
 	if params.Deprecated != nil {
 		util.FilterDevfileDeprecated(&index, *params.Deprecated, wantV1Index)
 	}
+	if index == nil || len(index) == 0 {
+		c.JSON(http.StatusOK, gin.H{


Suggested change

c.JSON(http.StatusOK, gin.H{

c.JSON(http.StatusInternalServerError, gin.H{

I'm wondering if this should be a 500 instead of a 200 as it not having the samples is technically an error and is referenced as an error in the original issue

https://go.dev/src/net/http/status.go

wdyt @michael-valdron

@Jdubrick Maybe 404 would be more appropriate error code here, since no stacks or samples can be found.

I was thinking 500 as it would be the servers fault no samples were loaded, no? I am also okay with 404 though

I was thinking 500 as it would be the servers fault no samples were loaded, no? I am also okay with 404 though

That is a good point, but in this case the control logic that provides this response does not guarantee that the stacks and samples are not loaded due to error versus just having an empty registry.

I'm okay with 404 in that case.

@Horiodino for reference that is http.StatusNotFound

IMO, in a world where we're working towards more dynamic devfile registries, I'd argue that a 200 code is fine here.

michael-valdron · 2024-06-11T16:58:12Z

index/server/pkg/server/endpoint.go

@@ -543,6 +543,12 @@ func buildIndexAPIResponse(c *gin.Context, indexType string, wantV1Index bool, p
 	if params.Deprecated != nil {
 		util.FilterDevfileDeprecated(&index, *params.Deprecated, wantV1Index)
 	}
+	if index == nil || len(index) == 0 {
+		c.JSON(http.StatusOK, gin.H{
+			"status": "No samples found in the registry",


Because this response is returned if index has no entries and not just the samples, the status should mention stacks too:

Suggested change

"status": "No samples found in the registry",

"status": "No stacks or samples found in the registry",

Signed-off-by: Horiodino <holiodin@gmail.com>

johnmcollier · 2024-06-13T20:25:12Z

index/server/pkg/server/endpoint.go

@@ -543,6 +543,12 @@ func buildIndexAPIResponse(c *gin.Context, indexType string, wantV1Index bool, p
 	if params.Deprecated != nil {
 		util.FilterDevfileDeprecated(&index, *params.Deprecated, wantV1Index)
 	}
+	if index == nil || len(index) == 0 {
+		c.JSON(http.StatusNotFound, gin.H{


TBH, I think this should actually be http.StatusOK.

@michael-valdron @Jdubrick It might be unusual to have a registry return no stacks, but IMO, I think 200 is more appropriate here than 404 or 500, especially as we work towards a more dynamic registry in devfile/api#1505

I'm okay with reverting it back to a 200 response. @Horiodino feel free to revert that change

Signed-off-by: Horiodino <holiodin@gmail.com>

Jdubrick

Tested locally - Removed extraDevfileEntries.yaml from the tests directory and created the registry. After hitting /index/sample I was able to receive the correct response.

@michael-valdron since you requested changes I'll wait until you take a look as well before merging

michael-valdron

/lgtm

Tried locally with an empty index, works as expected in headless:

@devfile/devfile-services-team I noticed that when the index empty that the registry viewer is throwing errors:

TypeError: devfileJsons[devfileRegistryIndex].map is not a function
    at /app/apps/registry-viewer/dist/.next/server/chunks/951.js:5066:110
    at Array.flatMap (<anonymous>)
    at fetchDevfiles (/app/apps/registry-viewer/dist/.next/server/chunks/951.js:5066:33)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async getServerSideProps (/app/apps/registry-viewer/dist/.next/server/pages/index.js:205:25)
    at async Object.renderToHTML (/app/node_modules/next/dist/server/render.js:507:20)
    at async doRender (/app/node_modules/next/dist/server/base-server.js:720:34)
    at async cacheEntry.responseCache.get.incrementalCache.incrementalCache (/app/node_modules/next/dist/server/base-server.js:837:28)
    at async /app/node_modules/next/dist/server/response-cache/index.js:83:36

If we are expecting a devfile registry to have a case with an empty index, there should be a follow issue created to address this on the registry viewer.

openshift-ci · 2024-07-11T16:33:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Horiodino, Jdubrick, michael-valdron

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:

~~OWNERS~~ [Jdubrick,michael-valdron]

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

michael-valdron · 2024-07-11T16:38:00Z

@Horiodino Changes looks good and merging this now, thanks for your contribution towards this!

Horiodino · 2024-07-12T17:31:24Z

@Horiodino Changes looks good and merging this now, thanks for your contribution towards this!

🙌 Looking forward to working more , thanks

fix #540

9896149

Signed-off-by: Horiodino <holiodin@gmail.com>

openshift-ci bot requested review from Jdubrick and michael-valdron June 11, 2024 15:13

Horiodino mentioned this pull request Jun 11, 2024

fixed: index server shouldn't return null if no samples exist #540 #233

Closed

3 tasks

openshift-ci bot added the ok-to-test label Jun 11, 2024

Jdubrick requested changes Jun 11, 2024

View reviewed changes

openshift-ci bot assigned Jdubrick Jun 11, 2024

michael-valdron requested changes Jun 11, 2024

View reviewed changes

openshift-ci bot assigned michael-valdron Jun 11, 2024

fix after review

f0a6a28

Signed-off-by: Horiodino <holiodin@gmail.com>

johnmcollier reviewed Jun 13, 2024

View reviewed changes

fxi after review

946f8d7

Signed-off-by: Horiodino <holiodin@gmail.com>

Horiodino requested review from Jdubrick, michael-valdron and johnmcollier July 9, 2024 17:29

Jdubrick approved these changes Jul 9, 2024

View reviewed changes

openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 9, 2024

michael-valdron approved these changes Jul 11, 2024

View reviewed changes

michael-valdron merged commit 4e34d7d into devfile:main Jul 11, 2024
8 checks passed

michael-valdron mentioned this pull request Jul 11, 2024

The index server shouldn't return null if no samples exist devfile/api#540

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #540 #246

fix #540 #246

Horiodino commented Jun 11, 2024 •

edited

Loading

Jdubrick commented Jun 11, 2024

Jdubrick commented Jun 11, 2024

Horiodino commented Jun 11, 2024

Horiodino commented Jun 11, 2024

Jdubrick commented Jun 11, 2024

Horiodino commented Jun 11, 2024

Jdubrick Jun 11, 2024

michael-valdron Jun 11, 2024

Jdubrick Jun 11, 2024

michael-valdron Jun 11, 2024

Jdubrick Jun 11, 2024

johnmcollier Jun 13, 2024

michael-valdron Jun 11, 2024

johnmcollier Jun 13, 2024

Jdubrick Jun 17, 2024

Jdubrick left a comment

michael-valdron left a comment •

edited

Loading

openshift-ci bot commented Jul 11, 2024

michael-valdron commented Jul 11, 2024

Horiodino commented Jul 12, 2024

	c.JSON(http.StatusOK, gin.H{
	c.JSON(http.StatusInternalServerError, gin.H{

	"status": "No samples found in the registry",
	"status": "No stacks or samples found in the registry",

fix #540 #246

fix #540 #246

Conversation

Horiodino commented Jun 11, 2024 • edited Loading

Jdubrick commented Jun 11, 2024

Jdubrick commented Jun 11, 2024

Horiodino commented Jun 11, 2024

Horiodino commented Jun 11, 2024

Jdubrick commented Jun 11, 2024

Horiodino commented Jun 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jdubrick left a comment

Choose a reason for hiding this comment

michael-valdron left a comment • edited Loading

Choose a reason for hiding this comment

openshift-ci bot commented Jul 11, 2024

michael-valdron commented Jul 11, 2024

Horiodino commented Jul 12, 2024

Horiodino commented Jun 11, 2024 •

edited

Loading

michael-valdron left a comment •

edited

Loading