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

Run config validation test in a different directory than the repo, fix bugs found when making the change #827

Merged
merged 9 commits into from
Jan 13, 2023

Conversation

heyealex
Copy link
Contributor

Description

This goal of this PR was to run the validate_configs.sh validation outside of the HPC Toolkit repo. Doing so exposed a bug with the packer modulereader package where the packerreader attempts to copy files by default, without checking if the source is local vs embedded.

The fix to this is included as well to ensure tests are passing again. Some refactoring took place as well with the sourcereader and modulereader. It became difficult to avoid cyclical dependencies, so I instead moved some functionality out of sourcereader and into modulereader where it belongs, and now use sourcereader more actively when copying a module from various sources to the file system.

There is still work to do to improve these modules (e.g. function names need to improve), but I tried to keep it limited to what was needed to reasonably get tests passing again without adding additional tech debt.

Changes made

Testing

  • make tests now also builds and installs the binary
  • Moved golang tests where applicable, I didn't actually rewrite any beyond making them work in a different module

Refactor

  • Reversed the package dependency between modulereader and sourcereader
    • Now the dependency is more linear: modulereader -> sourcereader -> blueprintio
    • Functionality of modulereader started slipping into sourcereader, this attempts to fix that, since it was getting difficult to avoid cyclical dependencies.
  • packerreader no longer copies the module to a temp directory itself, it relies on existing functionality in sourcereader
  • Moved API list and functionality to modulereader
  • Moved GetModuleInfo from sourcereader implementations to the top level of modulereader (it didn't belong in sourcereader, and this decreases duplicated code)

Submission Checklist

  • Have you installed and run this change against pre-commit? (pre-commit install)
  • Are all tests passing? (make tests)
  • Have you written unit tests to cover this change?
  • Is unit test coverage still above 80%?
  • Have you updated all applicable documentation?
  • Have you followed the guidelines in our Contributing document?

Shellcheck pushed me to moved from `grep ... | wc -l` to `grep -c ...`,
which did not work when trying to save the value to a variable in a
subshell. Instead, I switched to using `grep -q ...` directly in the
conditional.
@heyealex heyealex marked this pull request as ready for review January 11, 2023 18:03
Copy link
Member

@tpdownes tpdownes left a comment

Choose a reason for hiding this comment

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

This is a very significant improvement in the code and its technical debt. Please address the minor regex change and concern about the unit tests.

Additionally, I suggest renaming output.tf to outputs.tf to comply with standard module structure.

Once you make these changes, do you believe we should run all optional PR checks or ought they be unaffected?

tools/validate_configs/validate_configs.sh Outdated Show resolved Hide resolved
tools/validate_configs/validate_configs.sh Outdated Show resolved Hide resolved
pkg/modulereader/resreader_test.go Show resolved Hide resolved
@tpdownes tpdownes assigned heyealex and unassigned tpdownes Jan 12, 2023
Copy link
Member

@cboneti cboneti left a comment

Choose a reason for hiding this comment

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

Please see some of my comments here. I have not reviewed the go code, but I trust Tom went through it in detail.

Makefile Outdated Show resolved Hide resolved
tools/validate_configs/validate_configs.sh Outdated Show resolved Hide resolved
@tpdownes
Copy link
Member

Please see some of my comments here. I have not reviewed the go code, but I trust Tom went through it in detail.

The bulk of the changes to Go code are cosmetic due to the improved dependency structure. That said, there is some real action in resreader.go which I support. The mental path through the code is easier to traverse.

@heyealex heyealex assigned tpdownes and cboneti and unassigned heyealex Jan 12, 2023
@heyealex
Copy link
Contributor Author

I have integration tests running for another PR (#832), I will kick them off for this PR when they finish

@cboneti cboneti removed their assignment Jan 13, 2023
@heyealex heyealex dismissed tpdownes’s stale review January 13, 2023 19:00

Discussed at standup, it's good to merge

@heyealex heyealex merged commit 99d7bf9 into GoogleCloudPlatform:develop Jan 13, 2023
@heyealex heyealex deleted the tests/run-outside-repo branch January 13, 2023 19:00
@cboneti cboneti mentioned this pull request Jan 31, 2023
6 tasks
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.

3 participants