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

feat(manager): add sveltos manager #30087

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

oliverbaehler
Copy link

@oliverbaehler oliverbaehler commented Jul 8, 2024

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])

  • I have updated the documentation, or
  • No documentation update is required

Not sure if there's more?

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@CLAassistant

This comment was marked as resolved.

@secustor

This comment was marked as resolved.

@secustor secustor marked this pull request as draft July 8, 2024 17:18
@oliverbaehler

This comment was marked as resolved.

@secustor secustor marked this pull request as ready for review July 8, 2024 20:13
@secustor
Copy link
Collaborator

secustor commented Jul 8, 2024

Package manager details can be found here: #30090 (comment)

lib/modules/manager/sveltos/extract.spec.ts Outdated Show resolved Hide resolved
lib/modules/manager/sveltos/index.ts Outdated Show resolved Hide resolved
lib/modules/manager/sveltos/extract.ts Outdated Show resolved Hide resolved
lib/modules/manager/sveltos/extract.ts Outdated Show resolved Hide resolved
secustor

This comment was marked as resolved.

@oliverbaehler

This comment was marked as resolved.

@secustor

This comment was marked as resolved.

@oliverbaehler

This comment was marked as resolved.

@secustor secustor self-requested a review August 12, 2024 08:11
@secustor
Copy link
Collaborator

Is this ready for another review?
Please use the re-review button so we maintainers know that you are ready for a new review.

BTW It isn't necessary to merge main if there are no conflicts.

@oliverbaehler
Copy link
Author

@secustor Yes this is ready for review, Sorry!

lib/modules/manager/sveltos/extract.spec.ts Outdated Show resolved Hide resolved
lib/modules/manager/sveltos/readme.md Outdated Show resolved Hide resolved
lib/modules/manager/sveltos/readme.md Outdated Show resolved Hide resolved
lib/modules/manager/sveltos/readme.md Outdated Show resolved Hide resolved
lib/modules/manager/sveltos/readme.md Show resolved Hide resolved
lib/modules/manager/sveltos/readme.md Outdated Show resolved Hide resolved
lib/modules/manager/sveltos/readme.md Outdated Show resolved Hide resolved
@viceice viceice self-requested a review August 14, 2024 15:58
Copy link
Collaborator

@secustor secustor left a 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

lib/modules/manager/sveltos/util.ts Outdated Show resolved Hide resolved
lib/modules/manager/sveltos/schema.ts Outdated Show resolved Hide resolved
lib/modules/manager/sveltos/extract.ts Outdated Show resolved Hide resolved
lib/modules/manager/sveltos/extract.ts Outdated Show resolved Hide resolved
lib/modules/manager/sveltos/extract.ts Outdated Show resolved Hide resolved
lib/modules/manager/sveltos/extract.ts Outdated Show resolved Hide resolved
lib/modules/manager/sveltos/extract.ts Outdated Show resolved Hide resolved
Signed-off-by: Oliver Bähler <[email protected]>
lib/modules/manager/sveltos/extract.spec.ts Show resolved Hide resolved
lib/modules/manager/sveltos/extract.ts Outdated Show resolved Hide resolved
lib/modules/manager/sveltos/schema.ts Outdated Show resolved Hide resolved
lib/modules/manager/sveltos/extract.ts Outdated Show resolved Hide resolved
viceice
viceice previously approved these changes Aug 29, 2024
Copy link
Collaborator

@secustor secustor left a comment

Choose a reason for hiding this comment

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

You are missing coverage.

@oliverbaehler
Copy link
Author

Hi I am sorry I am unable to get the coverage up :( . Can I somehow create the coverage locally? What i currently do is run Jest:

pnpm jest lib/modules/manager/sveltos
...
Statements   : 2.39% ( 1023/42686 )
Branches     : 0.36% ( 49/13404 )
Functions    : 0.51% ( 27/5294 )
Lines        : 2.42% ( 1020/42083 )

But i don't know how to interpret these results..

@rarkins
Copy link
Collaborator

rarkins commented Sep 4, 2024

### Coverage
The Renovate project maintains 100% test coverage, so any Pull Request will fail if it does not have full coverage for code.
Using `// istanbul ignore` is not ideal, but can be a pragmatic solution if adding more tests wouldn't really prove anything.
To view the current test coverage locally, open up `coverage/index.html` in your browser.
Do not let coverage put you off submitting a PR!
Maybe we can help, or at least guide.
Also, it can be good to submit your PR as a work in progress (WIP) without tests first so that you can get a thumbs up from others about the changes, and write tests after.

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.

6 participants