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

Create exports with multiple sites #88

Merged
merged 11 commits into from
Jan 23, 2024
Merged

Create exports with multiple sites #88

merged 11 commits into from
Jan 23, 2024

Conversation

scotho3
Copy link
Contributor

@scotho3 scotho3 commented Nov 25, 2023

No description provided.

@scotho3 scotho3 mentioned this pull request Nov 25, 2023
@bortok
Copy link
Contributor

bortok commented Nov 26, 2023

I would prefer to make the CLI supporting both single and multiple sites, i.e:

existing syntax:

nrx.py --site SFO
nrx.py -s SFO

as well as the new one (long form only):

nrx.py --sites SFO,SJC,OAK

but make it so that the behavior is really the same: --site SFO,SJC should be valid, as well as --sites SFO.
This way there is no need to change any existing tests and examples, and more importantly, no users that use --site would be impacted.

Do you see any issues with implementing this? Could you add this to the PR?

@bortok bortok added the enhancement New feature or request label Nov 26, 2023
@bortok bortok added this to the WAN Topologies milestone Nov 26, 2023
@bortok bortok linked an issue Nov 26, 2023 that may be closed by this pull request
@bortok
Copy link
Contributor

bortok commented Nov 27, 2023

There is another UX angle that I realized when playing with the multi-site export. The name of the topology becomes long and not necessarily pretty. I believe there is a need for --name argument to explicitly provide one. What I'm going to do is create another PR before merging this one for --name and then the multi-site export will be able to take advantage of it. Hope you could look into adopting both --site and --sites meanwhile

@scotho3
Copy link
Contributor Author

scotho3 commented Dec 1, 2023

That works! I will modify my PR to use --sites

@bortok
Copy link
Contributor

bortok commented Dec 5, 2023

Hi @scotho3. I added --name parameter, please pull the latest updates from main. There are some conflicts you'd need to resolve.

@bortok
Copy link
Contributor

bortok commented Dec 7, 2023

I took the liberty to merge the latest changes with --name into your fork. Do you know when you could update the PR to use both --site and --sites interchangeably?

@bortok bortok merged commit dba9abb into netreplica:main Jan 23, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow multi-site export
2 participants