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

Move all Kinds into the package kinds #2244

Closed

Conversation

robertvolkmann
Copy link
Contributor

Continuation of #1944

@hellt
Copy link
Member

hellt commented Oct 17, 2024

Hi @robertvolkmann
thanks for spending time on this
what do you achieve with packing kinds under kinds/types?

@robertvolkmann
Copy link
Contributor Author

robertvolkmann commented Oct 17, 2024

You're right, I can also move them one level up. In the end, the registration of the kinds is no longer part of the clab package. I followed the idea from @steiler's draft. If it doesn't make sense, close the PR.

@hellt
Copy link
Member

hellt commented Oct 18, 2024

I think the end goal is twofold

  1. make clab package importable so that it can be called as an api (high level goal)
  2. make sure that the cmd pkg doesn't contain any specifics that drive the clab package

The clab package registering kinds is OK, because it does not prevent points 1 and 2 above to be achieved.

@steiler
Copy link
Collaborator

steiler commented Oct 18, 2024

Yes, so in #1944 the plan was to have a specific kind_registry that you can query with kind name and retrieve additional information about that kind. One of the issues was e.g. the creadentials, they are set as a global var in the package.
By creating the kind_registry the idea was to use that as a central point where the kinds would register themselfes and also provide additional infos that can be queried from it in the deployment or and other of the workflows.

Also in that PR I wanted to move the kind implementations into kinds/types which in my view made sense.
Having the registry and kind general stuff in the kinds folder and then push the specific types into the kinds/types folder.

In the End this is not the most essenstial step in providing clab as a proper library, which amongst others includes decoupling the cmd package from the rest, but this is definetely a step in the right direction.

Copy link

codecov bot commented Oct 18, 2024

Codecov Report

Attention: Patch coverage is 59.03614% with 68 lines in your changes missing coverage. Please review.

Project coverage is 51.32%. Comparing base (6859e39) to head (3cb9eac).

Files with missing lines Patch % Lines
kinds/registry.go 51.35% 15 Missing and 3 partials ⚠️
kinds/credentials.go 60.00% 7 Missing and 1 partial ⚠️
kinds/types/cvx/cvx.go 33.33% 2 Missing and 2 partials ⚠️
kinds/registry_builder.go 50.00% 2 Missing and 1 partial ⚠️
kinds/all/register.go 33.33% 1 Missing and 1 partial ⚠️
clab/config/utils.go 0.00% 1 Missing ⚠️
kinds/types/border0/border0.go 50.00% 0 Missing and 1 partial ⚠️
kinds/types/c8000/c8000.go 50.00% 0 Missing and 1 partial ⚠️
...pes/checkpoint_cloudguard/checkpoint_cloudguard.go 50.00% 0 Missing and 1 partial ⚠️
kinds/types/crpd/crpd.go 50.00% 0 Missing and 1 partial ⚠️
... and 28 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2244      +/-   ##
==========================================
- Coverage   52.12%   51.32%   -0.80%     
==========================================
  Files         172      174       +2     
  Lines       12714    16605    +3891     
==========================================
+ Hits         6627     8523    +1896     
- Misses       5174     7163    +1989     
- Partials      913      919       +6     
Files with missing lines Coverage Δ
clab/clab.go 69.55% <100.00%> (-0.67%) ⬇️
clab/dependency_manager/dependency_node.go 87.85% <ø> (+0.19%) ⬆️
clab/inventory.go 84.00% <100.00%> (-1.72%) ⬇️
clab/sshconfig.go 67.79% <100.00%> (+4.93%) ⬆️
cmd/generate.go 65.47% <ø> (-2.84%) ⬇️
kinds/types/bridge/bridge.go 60.97% <100.00%> (ø)
kinds/types/ceos/ceos.go 60.10% <100.00%> (ø)
kinds/types/ext_container/ext_container.go 80.55% <100.00%> (ø)
kinds/types/fortinet_fortigate/fortigate.go 84.09% <100.00%> (ø)
kinds/types/host/host.go 41.11% <100.00%> (ø)
... and 51 more

... and 105 files with indirect coverage changes

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.

3 participants