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

add BGP commands for enabling, getting, and session listing #177

Merged
merged 3 commits into from
Jun 17, 2022

Conversation

displague
Copy link
Member

@displague displague commented Mar 15, 2022

Adds commands for BGP enabling, BGP getting, and BGP session listing.

Not included in this PR:

In addition to the typical review, I'm looking for feedback on the following decision points:

  • Fields exposed and formatting
  • Documentation content and format
  • I wasn't sure if the best place for this would be metal bgp <command> or metal project bgp-<command>. I went with the latter, following the API structure. Device specific BGP commands would then land in metal device bgp-<command>.
  • I think I would prefer for the project-id to be defaulted to the metal init project id for these BGP commands, but I opted not to take on the default because other metal project commands do not.
    metal project commands tend to use --id rather than --project-id, which avoids the default project. This is done by choice to avoid having a default project for metal project delete.
    Looking for thoughts on this. If either metal project bgp* --project-id or metal bgp --project-id are preferrable we might buck some convention for usability (or confusion).

Examples of added functionality:

# Enable BGP on project 50693ba9-e4e4-4d8a-9eb2-4840b11e9375:
metal project bgp-enable --id 50693ba9-e4e4-4d8a-9eb2-4840b11e9375 --deployment-type local --asn 65000

# Get BGP config for project 50693ba9-e4e4-4d8a-9eb2-4840b11e9375:
metal project bgp-config --id 50693ba9-e4e4-4d8a-9eb2-4840b11e9375

# Get BGP Sessions for project 50693ba9-e4e4-4d8a-9eb2-4840b11e9375:
metal project bgp-sessions --id 50693ba9-e4e4-4d8a-9eb2-4840b11e9375

@displague displague force-pushed the bgp-enable branch 2 times, most recently from 969ace8 to 74f2666 Compare March 15, 2022 13:41

```
-h, --help help for bgp-config
-i, --id string Project ID (METAL_PROJECT_ID)
Copy link
Member Author

Choose a reason for hiding this comment

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

METAL_PROJECT_ID is only used when the argument is --project-id.

There are a few places in metal project where this environment variable is incorrectly suggested (including here).

Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm... do you know which those are, I've been trying to spot check it as I go along. Even better if we could make that behavior consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, do you mean there is some actual, meaningful distinction between when the flag is --id vs --project-id? Because that's....not great?

Copy link
Member Author

Choose a reason for hiding this comment

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

--id tends to be relative to the resource you are working with in each command.

--project-id is a standard, almost global, argument because it is so frequently needed.

Because --project-id inherits from the metal init config, to prevent the default project from being applied on all project commands (like delete) we avoided --project-id in the metal project set of commands. In favor of that, we use --id (the action relative parameter).

In any case, METAL_PROJECT_ID only maps to --project-id arguments, not --id.

When we start talking about metal project bgp* the dominant prevalence of Project and BGP is called into question when we think about --id.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmmmmm......ok. That makes more sense than what I was thinking. And now I see the conflict here. So is if it is ok to use --project-id for project ID and --id for the BGP id?

In this case, probably yes? because if --project-id is pulled in from the environment it won't supercede any other behavior. I think. Like there is nothing to be expected if you just send in metal bgp get. It would be ok for metal bgp get to just return the same thing as `metal bgp get --project-id <project_id>.

So I think I would use --project-id and --id here. Unless I am missing something.

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense.

@displague displague self-assigned this Apr 4, 2022
Copy link
Member Author

@displague displague left a comment

Choose a reason for hiding this comment

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

So I think I would use --project-id and --id here. Unless I am missing something.

I'll have to update this to change the project id parameter

Signed-off-by: Marques Johansson <mjohansson@equinix.com>
Signed-off-by: Marques Johansson <mjohansson@equinix.com>
Signed-off-by: Marques Johansson <mjohansson@equinix.com>
@displague
Copy link
Member Author

displague commented Jun 17, 2022

Updated the metal project bgp* commands to use --project-id rather than --id, which means the default metal init project is the default.

@cprivitere cprivitere merged commit 9296126 into main Jun 17, 2022
@cprivitere cprivitere deleted the bgp-enable branch June 17, 2022 16:17
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