-
Notifications
You must be signed in to change notification settings - Fork 3
Isolate docker interactions into module and simplify test docker containers #52
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
Isolate docker interactions into module and simplify test docker containers #52
Conversation
Thank you very much for taking the time to split up the pull request. These changes look a lot more manageable. I will add replies to this comment to collect more general questions and possible remaining issues that I find while checking the code that cannot be linked to a specific part of a file. First observations:
I will continue to review the code in iterations, but I am confident that we can apply most of the suggested changes as-is.
|
The format of the problems remains the same as in the 4.0 version, which afaik is the same as in 3.0.2, the bug you saw was caused by the input paths not being actual Path objects and me incorrectly assuming they were. the commit i just pushed should fix it. I can't observe the same behaviour regarding docker, it always exits right away for me. I also can't see why the code might behave that way, it raises |
a402a9b
to
ee5ad7b
Compare
After way more research and testing than I expected, I implemented the changes we discussed last week. The gist is that the docker module now uses the official docker api instead of shell commands. In addition I also moved the There are two more or less breaking changes here though:
There also are a couple further things of note: |
Thank you for commiting these changes, I will review them this week in detail and comment further here. A short test shows that the CI pipeline currently seems to fail, could you look into it? (I have not figured out yet why the automated CI tests are currently not launched in this PR) I do not mind the two breaking changes, they seem reasonable and since we are aiming for the next full release, such changes are to be expected. Please have an eye on not straying too far from the purpose of this specific pull request, I really appreciate your ideas and time to implement them, but it becomes much more complicated to properly review them if one pull request wants to do too many different things beyond its scope. |
CI runs here now (reason was that the |
I made #53 and #54 to split out the unrelated changes in here, if you merge them I can then either just rebase this branch to be based on them or also split out the other small and somewhat unrelated changes to the battle script (having it handle signals using the python error handling rather than the signal handler and having the functions definitions outside of the main function) and the teams objects (though that would be kinda messy since they are mainly about changing the way the battle script interacts with the docker containers) |
I have merged #53 and #54 into the release candidate. I would suggest not splitting out the changes to the teams object, as this is closely correlated to the docker changes and thus arguably in scope of this PR. If the required effort is reasonable I would advocate for splitting out the changes to the battle script (that are not related to the docker changes) to keep the PR clean. |
0630628
to
9086998
Compare
rebased the branch on the new 4.0 branch, file diffs are now properly only the docker changes. |
…xecuting the battle script from the algobattle folder directly
Thank you for taking the time to do these additional changes. I have reviewed the changes and think that they are helpful. For clarification - since I have not worked with python's Once this is cleared up, I am ready to merge these changes. |
The python |
Following your request of splitting up the changes in #49 (here) into multiple smaller PRs, this is the first of them where I tried to isolate all the interactions with docker into its own module and simplify the docker containers used for testing.
I also made some very minor modifications to related parts of the code, such as fixing up type annotations, passing the currently fighting teams as an argument to
FightHandler.fight()
instead of them being part of its state, and adding the flake8 config file.