-
Notifications
You must be signed in to change notification settings - Fork 120
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
fix: Update the tox.ini to different tox env #3164
base: main
Are you sure you want to change the base?
Conversation
Thanks for opening a Pull Request. If you want to perform a review write a comment saying: @ansys-reviewer-bot review |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3164 +/- ##
==========================================
- Coverage 87.44% 87.36% -0.09%
==========================================
Files 55 55
Lines 9845 9923 +78
==========================================
+ Hits 8609 8669 +60
- Misses 1236 1254 +18 |
a4fd62c
to
1c61a30
Compare
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.
This is going in the right direction, thanks @Revathyvenugopal162.
Co-authored-by: Jorge Martínez <28702884+jorgepiloto@users.noreply.github.com>
…to fix/improve-tox
bc25169
to
2a40945
Compare
docker run \ | ||
--env ANSYSLMD_LICENSE_FILE={env:ANSYSLMD_LICENSE_FILE} \ | ||
--name mapdl -p {env:LOCAL_MAPDL_PORT}:50052 \ |
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.
Thanks for the heads up, @germa89.
We should take advantage of this and add a rule for building a docker image locally. This will help us to review the docker/
directory and the current image building process.
This directory could look like this:
docker/
|- linux/
|- Dockerfile
|- docker-compose.yml
|- distributions/
|- .gitignore # Ignore everything except the dir itself
|- .gitkeep
Users should just copy MAPDL artifacts in the distributions/
dir. Then, running:
tox -e docker-build-images
would trigger the docker-compose.yml
file.
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.
Building the docker image is a bit complex.. because the dockerignore
depends on the MAPDL version.
The docker
directory is not used by the CICD nor myself to build the docker image at the moment, hence it is not fully maintained.
We have some instructions about building the docker image in: https://mapdl.docs.pyansys.com/version/stable/getting_started/make_container.html
We could meet next week to discuss this more in details.
Wiz Scan Summary
|
985f42f
to
58aaebb
Compare
@@ -1,84 +1,112 @@ | |||
# Use Ubuntu 22.04 as the base image |
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.
This line is explicit. No need for a comment.
# Use Ubuntu 22.04 as the base image |
docker/linux/Dockerfile
Outdated
&& rm -rf *.log | ||
|
||
# Cleaning up unnecessary files | ||
RUN rm -rf /tmp/* |
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.
Remove only the right files in the tmp/
dir. There may be files in this directory used by other running processes.
- USERNAME=mapdl | ||
- USER_UID=1000 | ||
- LICENSE_SERVER=${LICENSE_SERVER} | ||
- AWP_ROOT222=/ansys_inc |
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.
Is this a variable linked to a unified install? Should it be AWP_ROOT241
?
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.
it is.
GITHUB_TOKEN | ||
commands = | ||
docker login ghcr.io -u {env:GITHUB_USERNAME} -p {env:GITHUB_TOKEN} |
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.
Login is no longer required since users can build the images locally:
docker login ghcr.io -u {env:GITHUB_USERNAME} -p {env:GITHUB_TOKEN} |
--env ANSYSLMD_LICENSE_FILE={env:ANSYSLMD_LICENSE_FILE} \ | ||
--name mapdl -p {env:LOCAL_MAPDL_PORT}:50052 \ | ||
{env:MAPDL_DOCKER_REGISTRY_URL} -smp |
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.
{env:MAPDL_DOCKER_REGISTRY_URL} -smp | |
-smp |
@jorgepiloto @Revathyvenugopal162 how are we doing with this? :) |
Sorry for the delay, I was tied up with the theme reformatting. I’ll dive into this PR next week, fix the issue, and keep you updated on the progress. |
Hello! 👋 Your PR is changing the image cache. So I am attaching the new image cache in a new commit. This commit does not re-run the CICD workflows (since no changes are made in the codebase) therefore you will see the actions showing in their status You might want to rerun the test to make sure that everything is passing. You can retrigger the CICD sending an empty commit You will see this message everytime your commit changes the image cache but you are not attaching the updated cache. 🤓 |
No description provided.