-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
feat(manager): add sveltos manager #30087
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Package manager details can be found here: #30090 (comment) |
c7b2ba4
to
979cd3d
Compare
36f2170
to
fcaf6ef
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Signed-off-by: Oliver Bähler <[email protected]>
e0ca135
to
2d79c49
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Is this ready for another review? BTW It isn't necessary to merge main if there are no conflicts. |
@secustor Yes this is ready for review, Sorry! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs code coverage ( see the failing CI ) and docs changes requested by @HonkingGoose
Co-authored-by: HonkingGoose <[email protected]>
Co-authored-by: HonkingGoose <[email protected]>
Co-authored-by: HonkingGoose <[email protected]>
Co-authored-by: HonkingGoose <[email protected]>
Co-authored-by: HonkingGoose <[email protected]>
HI @secustor, I implemented your changes. I also moved the fixtures from the
Lint also throws errors:
DO I need to update/change something? |
Looks like you forgot to install the updated dependencies, after merging in main. |
@secustor Oh thanks! Sorry really not trying to waste everyone's time.. 3: |
Co-authored-by: HonkingGoose <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs prettier fix, else LGTM
@secustor thanks, hope it's now fixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution!
🎉 This PR is included in version 38.124.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
@secustor Thanks for merging, just a quick question: Does it take some time to update the version on developer.mend.io? It keeps telling me that the configuration with the sveltos key is invalid 🤔:
|
The hosted app is not updated automatically. Not sure when Mend will update the hosted app again. |
When is the hosted app updated?Full quote from Renovate docs, when is the Mend Renovate app updated with new Renovate versions?:
How to see which Renovate version you're onRead Renovate docs, How can I see which version the Mend Renovate app is using? to learn how to find the logs, and how to find the Renovate version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for late comments
export function extractDefinition( | ||
definition: ProfileDefinition, | ||
config?: ExtractConfig, | ||
): PackageDependency[] { | ||
return processAppSpec(definition, config); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wonder why we need this simple wrapper?
}; | ||
|
||
if (isOCIRegistry(source.repositoryURL)) { | ||
const image = trimTrailingSlash(removeOCIPrefix(source.repositoryURL)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why trim trailing slash? doesn't make sense to me
return dep; | ||
} | ||
|
||
function processAppSpec( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function should be inlined
const repoNameWithSlash = regEx(`^${repositoryName}/`, undefined, false); | ||
let modifiedChartName = chartName.replace(repoNameWithSlash, ''); | ||
|
||
modifiedChartName = modifiedChartName.replace(/\/+$/, ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, makes no sense and has no tests.
chartName: string, | ||
): string { | ||
const repoNameWithSlash = regEx(`^${repositoryName}/`, undefined, false); | ||
let modifiedChartName = chartName.replace(repoNameWithSlash, ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let modifiedChartName = chartName.replace(repoNameWithSlash, ''); | |
let modifiedChartName = chartName.substring(repoName.length). replace (regEx('^/'), ''); |
why not even simpler?
Changes
This adds the manager for sveltos for GitOps managed Cluster Fleets. Essentially we have the same functionality as argoCD, so we were able to take the argocd manager and adjust the schema etc. I hope that's okay.
Context
Documentation (please check one with an [x])
Not sure if there's more?
How I've tested my work (please select one)
I have verified these changes via: