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 nonkube connector CLI #1713

Merged
merged 4 commits into from
Nov 8, 2024

Conversation

lynnemorrison
Copy link
Collaborator

@lynnemorrison lynnemorrison commented Oct 11, 2024

Add Connector CLI commands for nonkube implementation
includes create, update status and delete.

@lynnemorrison lynnemorrison self-assigned this Oct 11, 2024
@lynnemorrison
Copy link
Collaborator Author

lynnemorrison commented Oct 11, 2024

@nluaces
For unit tests I created a dummy test file and then deleted it after test. Want to verify this is acceptable.
.

@lynnemorrison lynnemorrison force-pushed the v2_nonkube_connector branch 2 times, most recently from fc6e23f to d39bbc3 Compare October 14, 2024 18:53
@lynnemorrison lynnemorrison force-pushed the v2_nonkube_connector branch 4 times, most recently from af796c4 to 780e06a Compare October 22, 2024 20:48
@nluaces
Copy link
Member

nluaces commented Oct 23, 2024

timeout flag is specified in refdog for nonkube but not sure how that is used.
The timeout flag should be not used for now, giving that there is no controller for non kube environments. I suggest to hide it for the non_kube implementations.

Regarding the custom resource handler interface, remember to write implementations (empty if it is necessary) to all the resources that implement the interface (i.e. if you implement the List method for connectors, you also need to create the List method in sites)

@lynnemorrison lynnemorrison force-pushed the v2_nonkube_connector branch 5 times, most recently from fbe53fe to 4484e07 Compare October 28, 2024 14:25
@lynnemorrison lynnemorrison force-pushed the v2_nonkube_connector branch 2 times, most recently from e51a57a to 5f216e2 Compare October 28, 2024 16:08
@lynnemorrison lynnemorrison force-pushed the v2_nonkube_connector branch 3 times, most recently from ed56ee1 to b61e425 Compare October 30, 2024 15:19
@lynnemorrison lynnemorrison force-pushed the v2_nonkube_connector branch 3 times, most recently from ca402b4 to acdc07c Compare November 4, 2024 22:12

// Validate that there is already a connector with this name in the namespace
if cmd.connectorName != "" {
connector, err := cmd.connectorHandler.Update(cmd.connectorName)
Copy link
Member

Choose a reason for hiding this comment

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

This method is called Update, but it is actually just retrieving a connector.
Should it be named something else?

Copy link
Member

Choose a reason for hiding this comment

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

I missed this, I agree that Update should update a resource.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will add new issue to fix

return connectors, nil
}

func (s *ConnectorHandler) Update(name string) (*v2alpha1.Connector, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Update does not seem like an appropriate name for what this method is doing.
Maybe we could tweak the Get signature allowing some options to be passed
that indicates if it must read from runtime first or from input only?

Copy link
Member

Choose a reason for hiding this comment

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

Following the go client for kubernetes as example, the get command interface could be modified to something like this:

Get(name string, opts GetOptions) T

type GetOptions struct {
// runtimeFirst bool
// ...
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will add a new issue to modify the Get command.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

internal/nonkube/client/fs/path_provider.go Outdated Show resolved Hide resolved
internal/nonkube/client/fs/path_provider.go Outdated Show resolved Hide resolved
Copy link
Member

@fgiorgetti fgiorgetti left a comment

Choose a reason for hiding this comment

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

Other than my comments and minor suggestions above, it is working just fine.
Thank you!

@nluaces nluaces merged commit c6748fc into skupperproject:v2 Nov 8, 2024
1 check passed
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