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

[VPC-3359] Implemented vpc peering methods #1532

Merged
merged 17 commits into from
May 30, 2024

Conversation

guptado
Copy link
Contributor

@guptado guptado commented May 16, 2024

VPC Peering is a closed beta feature and not available for public use.

Testing doc

commands/displayers/vpc_peering.go Outdated Show resolved Hide resolved
commands/vpc_peerings.go Outdated Show resolved Hide resolved
commands/vpc_peerings.go Show resolved Hide resolved
commands/vpc_peerings.go Outdated Show resolved Hide resolved
commands/vpc_peerings_test.go Show resolved Hide resolved
do/vpcs.go Outdated Show resolved Hide resolved
do/vpcs.go Outdated Show resolved Hide resolved
commands/vpc_peerings.go Outdated Show resolved Hide resolved
commands/vpc_peerings.go Outdated Show resolved Hide resolved
commands/vpc_peerings.go Outdated Show resolved Hide resolved
commands/vpc_peerings.go Outdated Show resolved Hide resolved
commands/vpc_peerings.go Outdated Show resolved Hide resolved
commands/vpc_peerings.go Show resolved Hide resolved
commands/vpc_peerings.go Outdated Show resolved Hide resolved
args.go Outdated Show resolved Hide resolved
guptado and others added 7 commits May 17, 2024 19:07
Co-authored-by: ddatta-do <147642663+ddatta-do@users.noreply.github.com>
Co-authored-by: ddatta-do <147642663+ddatta-do@users.noreply.github.com>
Co-authored-by: ddatta-do <147642663+ddatta-do@users.noreply.github.com>
Co-authored-by: ddatta-do <147642663+ddatta-do@users.noreply.github.com>
Co-authored-by: ddatta-do <147642663+ddatta-do@users.noreply.github.com>
commands/vpc_peerings.go Outdated Show resolved Hide resolved
args.go Outdated Show resolved Hide resolved
commands/doit.go Outdated Show resolved Hide resolved
commands/vpc_peerings.go Show resolved Hide resolved
commands/vpc_peerings.go Outdated Show resolved Hide resolved
commands/vpc_peerings.go Show resolved Hide resolved
commands/vpc_peerings.go Outdated Show resolved Hide resolved
Copy link
Member

@bentranter bentranter left a comment

Choose a reason for hiding this comment

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

A few comments around documentation, and two larger design comments that I'll put here:

  1. I would prefer that the new command be under just peerings instead of vpc-peerings. Having it under vpc-peerings makes it repetitive to type out when invoking the command – doctl vpcs vpc-peerings --help, vs. doctl vpcs peerings --help for example.
  2. For the create subcommand, both arguments are flags. For a create command in doctl, the more common pattern is for the name to be a positional argument, and then other arguments to be flags. This would mean the create command is doctl vpcs peerings create <name> --vpc-ids <ids> instead.

(Pinging @andrewsomething and @danaelhe to see if they agree with the two design related points)

commands/vpc_peerings.go Outdated Show resolved Hide resolved
commands/vpc_peerings.go Outdated Show resolved Hide resolved
commands/vpc_peerings.go Outdated Show resolved Hide resolved
commands/vpc_peerings.go Outdated Show resolved Hide resolved
commands/vpc_peerings.go Outdated Show resolved Hide resolved
commands/vpc_peerings.go Outdated Show resolved Hide resolved
commands/vpc_peerings.go Outdated Show resolved Hide resolved
Copy link
Member

@bentranter bentranter left a comment

Choose a reason for hiding this comment

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

Looks great, awesome work!

@bentranter bentranter merged commit 42fc8b6 into digitalocean:main May 30, 2024
7 checks passed
Copy link

@AungkoMCM AungkoMCM left a comment

Choose a reason for hiding this comment

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

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.

None yet

7 participants