Skip to content

feat: sparse checkout for chart#942

Open
dennisvankekem wants to merge 4 commits intomainfrom
APL-1679
Open

feat: sparse checkout for chart#942
dennisvankekem wants to merge 4 commits intomainfrom
APL-1679

Conversation

@dennisvankekem
Copy link
Contributor

@dennisvankekem dennisvankekem commented Mar 9, 2026

chartsPath?: string,
): Promise<any | null> {
const uuid = uuidv4()
const helmChartsDir = `/tmp/otomi/charts/${catalogName}/${branch}/chart/${uuid}`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would merge getBYOWorkloadCatalogChart and fetchCatalogChart into a single function. getBYOWorkloadCatalogChart only constructs the helmChartsDir path using a UUID and passes it to fetchCatalogChart, which is not used anywhere else.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, the original functions only have it this way so that the v1 endpoint still works. For this new endpoint the functions can be merged.

$ref: '#/components/schemas/AplCatalogChartResponse'
'400':
$ref: '#/components/responses/BadRequest'
content:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Both ref and inline content for this error response seem problematic, we should consider returning only the ref.

        '400':
          $ref: '#/components/responses/BadRequest'

content:
application/json:
schema:
$ref: '#/components/schemas/OpenApiValidationError'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This endpoint might return a 200 OK with a null body instead of a 404, we should consider adding a 404 error response.

return true
}

export async function sparseCheckoutChart(
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems this function is unused.


const chartDir = checkoutResult.checkoutPath

// RBAC check if needed
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should consider removing this RBAC check:

  • We use catalog-level RBAC rather than chart-level RBAC,
  • This check will almost certainly fail for single-chart checkout since rbac.yaml won't be fetched with sparse checkout, causing both resolvedHelmChartsDir and fallbackDir to always return {}.

export async function fetchWorkloadCatalogChart(
url: string,
helmChartsDir: string,
branch: string = 'main',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the 3rd param has a default value of main, it should be moved to the last position of the required params, otherwise the 4th param becomes required and callers can't skip the 3rd param to use its default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants