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.

Sign up for GitHub

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

Jump to bottom

aeon: added the ability to connect from the config #1094

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

aeon: added the ability to connect from the config #1094

wants to merge 9 commits into from

Conversation

AlexandrLitkevich
Copy link
Contributor

@AlexandrLitkevich AlexandrLitkevich commented Jan 31, 2025 •
edited
Loading

aeon: added the ability to connect from the config

Closes #TNTP-1073

@AlexandrLitkevich AlexandrLitkevich changed the title aeon:added the ability to connect from the config Closes #TNTP-1073 aeon: added the ability to connect from the config Closes #TNTP-1073 Jan 31, 2025
@AlexandrLitkevich AlexandrLitkevich changed the title aeon: added the ability to connect from the config Closes #TNTP-1073 aeon: added the ability to connect from the config Feb 1, 2025
@AlexandrLitkevich AlexandrLitkevich force-pushed the TNTP-1073 branch 3 times, most recently from 661d0a3 to b16f839 Compare February 1, 2025 18:06
patapenka-alexey
patapenka-alexey reviewed Feb 3, 2025
View reviewed changes
cli/cmd/aeon.go Outdated
func newAeonConnectCmd() *cobra.Command {
var aeonCmd = &cobra.Command{
Use: "connect URI",
Short: "Connect to the aeon instance",
Long: `Connect to the aeon instance.
tt aeon connect localhost:50051
tt aeon connect unix://<socket-path>`,
tt aeon connect unix://<socket-path>
tt aeon connect -c path instanceName>`,
Copy link
Contributor

@patapenka-alexey patapenka-alexey Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
tt aeon connect -c path instanceName>`,
tt aeon connect -c /path/to/config -i INSTANCE_NAME`,

To correlate with tt connect/play help style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I corrected it.

patapenka-alexey
patapenka-alexey reviewed Feb 3, 2025
View reviewed changes
cli/cmd/aeon.go Outdated

us, ok := uri.(string)
if !ok {
return fmt.Errorf("it is impossible to result in a string")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("it is impossible to result in a string")
return errors.New("it is impossible to cast result in a string")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

patapenka-alexey
patapenka-alexey reviewed Feb 3, 2025
View reviewed changes
Copy link
Contributor

@patapenka-alexey patapenka-alexey left a comment

Choose a reason for hiding this comment

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

Thank you for the patch. Could you please add an Changelog entry?

oleg-jukovec
oleg-jukovec requested changes Feb 3, 2025
View reviewed changes
Copy link
Contributor

@oleg-jukovec oleg-jukovec left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  1. Let's change the interface a bit, see the comment below.
  2. Let's add parsing of TLS options from an instance configuration.
        advertise:
          params:
            transport: 'ssl'
            ssl_key_file: 'foo'
            ...
  1. We need tests for new functionality.

cli/cmd/aeon.go Outdated Show resolved Hide resolved
oleg-jukovec
oleg-jukovec requested changes Feb 4, 2025
View reviewed changes
Copy link
Contributor

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

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

  1. Let's add parsing of TLS options from an instance configuration.
        advertise:
          params:
            transport: 'ssl'
            ssl_key_file: 'foo'
            ...
  1. We need tests for new functionality.

@oleg-jukovec oleg-jukovec added the full-ci Enables full ci tests label Feb 4, 2025
patapenka-alexey
patapenka-alexey reviewed Feb 6, 2025
View reviewed changes
cli/cmd/aeon.go Outdated
tt aeon connect unix://<socket-path>`,
tt aeon connect localhost:50051
tt aeon connect unix://<socket-path>
tt aeon connect /path/to/config INSTANCE_NAME>`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
tt aeon connect /path/to/config INSTANCE_NAME>`,
tt aeon connect /path/to/config INSTANCE_NAME>`,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

patapenka-alexey
patapenka-alexey reviewed Feb 6, 2025
View reviewed changes
cli/cmd/aeon.go Outdated
@@ -26,11 +30,12 @@ var connectCtx = aeoncmd.ConnectCtx{

func newAeonConnectCmd() *cobra.Command {
var aeonCmd = &cobra.Command{
Use: "connect URI",
Use: "connect URI or connect PATH instance",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Use: "connect URI or connect PATH instance",
Use: "tt aeon connect [ URI | /path/to/config INSTANCE_NAME ] [flags]",

Maybe something like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks very good, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patapenka-alexey patapenka-alexey patapenka-alexey left review comments

@oleg-jukovec oleg-jukovec oleg-jukovec requested changes

@dmyger dmyger dmyger approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
full-ci Enables full ci tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AlexandrLitkevich @dmyger @oleg-jukovec @patapenka-alexey