fix: missing acl schema #888
Conversation
|
based on the assumption that clusterDomainSuffix is always present in settings |
ferruhcihan
left a comment
ferruhcihan
left a comment
There was a problem hiding this comment.
Overall, the approach looks good 👍 I just noticed that some tests are failing in workloadUtils.test.ts, and there’s a small improvement needed in the URL checks instead of exact match.
| : normalizeRepoUrl(repositoryUrl, isPrivate, isSSH) | ||
|
|
||
| const repoUrl = | ||
| repositoryUrl === `https://gitea.${cluster?.domainSuffix}` |
There was a problem hiding this comment.
This check would never match any actual repository URL and will cause all internal Gitea repositories to fail the equality check. For the URL check, we could use something like the following:
const isInternalGitea = (() => {
try {
const url = new URL(repositoryUrl)
return url.hostname === `gitea.${cluster?.domainSuffix}`
} catch {
return false
}
})()
const repoUrl = isInternalGitea
? repositoryUrl
: normalizeRepoUrl(repositoryUrl, isPrivate, isSSH)
There was a problem hiding this comment.
I will add an isInternalGiteaURL function and replace the duplicated checks with this function call. I wil also make a test for it
| post: | ||
| operationId: getWorkloadCatalog | ||
| x-eov-operation-handler: v1/workloadCatalog | ||
| x-aclSchema: Workload |
There was a problem hiding this comment.
Why schema is called workload if the resource is tWorkloadCatalog?
There was a problem hiding this comment.
WorkloadCatalog is part of the Workload workflow
| if (!existsSync(localHelmChartsDir)) mkdirSync(localHelmChartsDir, { recursive: true }) | ||
| let gitUrl = helmChartCatalogUrl | ||
| if (isGiteaURL(helmChartCatalogUrl)) { | ||
| if (helmChartCatalogUrl === `https://gitea.${clusterDomainSuffix}`) { |
There was a problem hiding this comment.
you could also rely on HELM_CHART_CATALOG env
There was a problem hiding this comment.
good tip! I will take it into account
There was a problem hiding this comment.
Pull request overview
This PR addresses bug APL-1412 by adding missing ACL schema declarations and replacing the isGiteaURL helper function with a direct string comparison against the cluster domain suffix.
- Adds
x-aclSchema: Workloadto three API endpoints that were missing ACL schema declarations - Replaces
isGiteaURL()function calls with explicit URL comparison usingclusterDomainSuffixparameter - Threads the
clusterDomainSuffixparameter through the workload utility functions
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/openapi/api.yaml | Adds missing x-aclSchema: Workload declarations to workload catalog endpoints |
| src/utils/workloadUtils.ts | Updates URL validation from isGiteaURL() to explicit domain suffix comparison |
| src/otomi-stack.ts | Passes cluster?.domainSuffix to workload functions and normalizes import ordering |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!existsSync(localHelmChartsDir)) mkdirSync(localHelmChartsDir, { recursive: true }) | ||
| let gitUrl = helmChartCatalogUrl | ||
| if (isGiteaURL(helmChartCatalogUrl)) { | ||
| if (helmChartCatalogUrl === `https://gitea.${clusterDomainSuffix}`) { |
There was a problem hiding this comment.
The URL comparison will fail when clusterDomainSuffix is undefined. The condition should check if clusterDomainSuffix exists before comparing, or use startsWith to match the URL pattern more robustly.
| if (helmChartCatalogUrl === `https://gitea.${clusterDomainSuffix}`) { | |
| if (clusterDomainSuffix && helmChartCatalogUrl.startsWith(`https://gitea.${clusterDomainSuffix}`)) { |
| if (!existsSync(helmChartsDir)) mkdirSync(helmChartsDir, { recursive: true }) | ||
| let gitUrl = url | ||
| if (isGiteaURL(url)) { | ||
| if (url === `https://gitea.${clusterDomainSuffix}`) { |
There was a problem hiding this comment.
The URL comparison will fail when clusterDomainSuffix is undefined. The condition should check if clusterDomainSuffix exists before comparing, or use startsWith to match the URL pattern more robustly.
| if (url === `https://gitea.${clusterDomainSuffix}`) { | |
| if (clusterDomainSuffix && url.startsWith(`https://gitea.${clusterDomainSuffix}`)) { |
| const repoUrl = | ||
| repositoryUrl === `https://gitea.${cluster?.domainSuffix}` |
There was a problem hiding this comment.
When cluster?.domainSuffix is undefined, this creates the string 'https://gitea.undefined' which will cause incorrect URL matching. Add a guard to check cluster?.domainSuffix exists before string interpolation.
| const repoUrl = | |
| repositoryUrl === `https://gitea.${cluster?.domainSuffix}` | |
| const giteaUrl = cluster?.domainSuffix ? `https://gitea.${cluster.domainSuffix}` : undefined | |
| const repoUrl = | |
| giteaUrl && repositoryUrl === giteaUrl |
tracks this bug: https://track.akamai.com/jira/browse/APL-1412