-
Notifications
You must be signed in to change notification settings - Fork 54
use scenario filenames in list scenarios when using a remote scenario
#401
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
use scenario filenames in list scenarios when using a remote scenario
#401
Conversation
a2f2c52 to
0c66be6
Compare
src/warnet/server.py
Outdated
| additional_args: list[str], | ||
| network: str = "warnet", | ||
| ) -> str: | ||
| scenario_path = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I believe we can remove scenario_path = None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in d50ba92
0c66be6 to
1410aa1
Compare
|
I like this overall and it works well with #384: https://github.com/mplsgrant/warnet/actions/runs/9844946239/job/27179455585#step:8:361 However, I'll need to rerun with the logging changes to be sure. |
1410aa1 to
55e4411
Compare
Great thanks! I was just trying to address your comment when I spotted that we duplicated most of the code in our two scenario run functions, so refactored that in the push I just made: |
pinheadmz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
concept ACK, maybe a nice improvement or two suggested
src/warnet/server.py
Outdated
| # Extract just the filename without path and extension | ||
| base_name = os.path.splitext(os.path.basename(scenario_name))[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes more sense to do this in cli/scenarios.py and just pass the name to the server. Could even theoretically be an option to add a custom --name when running a scenario. for example if you want to run the same scenario a few times in parallel.
warcli scenarios stop could even accept that name as an option instead of a pid !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in d50ba92
55e4411 to
d50ba92
Compare
d50ba92 to
ac4e558
Compare
|
post-merge ACK, nice looks good |
Fixes #393