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

Input file fix #123

Merged
merged 3 commits into from
Feb 18, 2021
Merged

Input file fix #123

merged 3 commits into from
Feb 18, 2021

Conversation

marcomangano
Copy link
Contributor

Purpose

Closes #122 . I uploaded on mfile a new tar.gz folder without subfolders, and changed the download bash script to untar directly into input_files. All the tests have been updated to point to this folder.

Note: the new tar.gz also includes a new .cgns file used for the rotating frame adjoint test added with #121

Type of change

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (non-backwards-compatible fix or feature)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Documentation update
  • Maintenance update
  • Other (please describe)

Testing

The tests have been checked locally.

Checklist

  • [NA] I have run flake8 and black to make sure the code adheres to PEP-8 and is consistently formatted
  • I have run unit and regression tests which pass locally with my changes
  • I have added new tests that prove my fix is effective or that my feature works
  • I have added necessary documentation

ewu63
ewu63 previously approved these changes Feb 18, 2021
@ewu63
Copy link
Collaborator

ewu63 commented Feb 18, 2021

@marcomangano did you also include the new cgns files for your rotation tests?

@@ -4,4 +4,4 @@

DIR=$(dirname $0)
wget -O $DIR/input_files.tar.gz http://umich.edu/~mdolaboratory/repo_files/ADflow/adflow_input_files.tar.gz
tar -xzf $DIR/input_files.tar.gz -C $DIR/../
tar -xzf $DIR/input_files.tar.gz -C $DIR
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems that the new file is a tarbomb. I personally think we should refrain from that types of behavior.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah okay that's fair, I just didn't like relying on the name of the directory stored in the tarball instead of the parent directory of the get-input-files.sh script when extracting, since it's harder to be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh really? Do you prefer to place a subfolder in the input_files folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it was safe because we were already opening it into a separate folder

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think Eirikur is in general against tarbombs which is fair, although in this case the script specifically extracts to a designated directory.

@marcomangano
Copy link
Contributor Author

@marcomangano did you also include the new cgns files for your rotation tests?

Yes!

@ewu63 ewu63 merged commit 3bb39d7 into mdolab:master Feb 18, 2021
@marcomangano marcomangano deleted the inputFile-fix branch February 24, 2021 11:31
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.

inputFiles directory name is inconsistent
3 participants