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

[v2][non-kube] site create command and CLI adaptation #1563

Merged
merged 38 commits into from
Sep 13, 2024

Conversation

nluaces
Copy link
Member

@nluaces nluaces commented Jul 31, 2024

  • Kube and non-kube implementations are available with the --platform at execution level to avoid dependency on the generic configuration only (through environment variables).
  • In non-kube platforms, the custom resources will be stored on a path provided by the component PathProvider. If the user does not provide a namespace (with --namespace), they will be created in the default namespace.

@nluaces nluaces added the do-not-merge Work In Progress label Jul 31, 2024
@nluaces nluaces self-assigned this Jul 31, 2024
@nluaces nluaces marked this pull request as draft July 31, 2024 17:50
@nluaces nluaces force-pushed the cli-factory branch 2 times, most recently from c04b5b6 to b50248c Compare August 19, 2024 12:24
@nluaces nluaces changed the title [v2][non-kube] site commands (WIP) [v2][non-kube] site create command and CLI adaptation Aug 19, 2024
@nluaces nluaces removed the do-not-merge Work In Progress label Aug 19, 2024
@nluaces nluaces marked this pull request as ready for review August 19, 2024 14:22
@nluaces nluaces requested a review from ajssmith August 19, 2024 19:26
@nluaces nluaces force-pushed the cli-factory branch 2 times, most recently from 6611375 to fdbace0 Compare August 21, 2024 18:13
Copy link
Collaborator

@lynnemorrison lynnemorrison left a comment

Choose a reason for hiding this comment

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

The code looks really good!!

@nluaces nluaces force-pushed the cli-factory branch 2 times, most recently from 731437c to df62748 Compare August 28, 2024 15:14
@nluaces
Copy link
Member Author

nluaces commented Sep 4, 2024

A few highlights that may be helpful to review this pull request:

  • Refactor: the cobra command is defined outside the implementation of the different platforms, one cobra command will have available 2 implementations (kube and nonkube) to have both of them available at execution time. This change implied the modification of the test suites.

  • Nonkube command implementation: There is only one nonkube command implemented, and it is the skupper site create command, the rest will be implemented on following pull requests

  • New package fs (filesystem):

    • PathProvider: determines the path where the custom resource files should be located, .local/share/skupper/default/prepared/state by default.
    • CustomResourceHandler: this is the interface that will implement the rest of the handlers defined for each custom resource. For example, SiteHandler implements CustomResourceHandler interface and writes and stores the file in the correct directory (.local/share/skupper/default/prepared/state/sites)

That way the skupper command is decoupled from the logic of managing custom resources in a nonkube way.

Copy link
Member

@ajssmith ajssmith left a comment

Choose a reason for hiding this comment

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

built and used against kind cluster various cli operations

lgtm

Copy link
Contributor

@c-kruse c-kruse left a comment

Choose a reason for hiding this comment

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

🥳

@nluaces
Copy link
Member Author

nluaces commented Sep 13, 2024

@fgiorgetti let me know if something is missing before I merge this pull request. Thanks!

@fgiorgetti
Copy link
Member

@fgiorgetti let me know if something is missing before I merge this pull request. Thanks!

Thank you very much @nluaces for addressing all the comments.

@nluaces nluaces merged commit f1974d2 into skupperproject:v2 Sep 13, 2024
1 check passed
@nluaces nluaces deleted the cli-factory branch February 5, 2025 15:34
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