Skip to content

fix: shellcheck errors - #298

Merged
harryskim merged 2 commits into
ai-dynamo:mainfrom
csicar:fix/shellcheck-warnings
Apr 10, 2025
Merged

harryskim merged 2 commits into
ai-dynamo:mainfrom
csicar:fix/shellcheck-warnings

Conversation

@csicar

@csicar csicar commented Mar 19, 2025

Copy link
Copy Markdown
Contributor

Overview:

This MR improves the ./container/*.sh scripts.

Details:

The changes include:

  • Fix typo
  • Use proper /usr/bin/env interpreter
  • Quote variable where applicable
  • Remove double negation in if statements

Where should the reviewer start?

Take a look at ./container/*.sh

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

no related issue was found, but e.g. on NixOS, you cannot run these scripts, since bash is not located under /bin/bash

@copy-pr-bot

copy-pr-bot Bot commented Mar 19, 2025

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions

Copy link
Copy Markdown
Contributor

👋 Hi csicar! Thank you for contributing to ai-dynamo/dynamo.

Just a reminder: The NVIDIA Test Github Validation CI runs an essential subset of the testing framework to quickly catch errors.Your PR reviewers may elect to test the changes comprehensively before approving your changes.

🚀

@csicar
csicar force-pushed the fix/shellcheck-warnings branch from 86dd9ae to b89e82c Compare March 19, 2025 17:50
@csicar csicar changed the title Fix shellcheck errors fix: shellcheck errors Mar 19, 2025
Comment thread container/run.sh Outdated
Comment thread container/run.sh Outdated
Comment thread container/run.sh Outdated
Comment thread container/run.sh Outdated
Comment thread container/build.sh Outdated

@rmccorm4 rmccorm4 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM overall, just a few minor comments. Thanks for the change! We can consider adding shellcheck to our automation to avoid this being a recurring change.

@rmccorm4 rmccorm4 added the external-contribution Pull request is from an external contributor label Mar 20, 2025
@csicar
csicar force-pushed the fix/shellcheck-warnings branch from b89e82c to 0cf298b Compare March 20, 2025 09:13
- Fix typo
- Use proper `/usr/bin/env` interpreter
- Quote variable where applicable
- Remove double negation in if statements
- and much more
@csicar
csicar force-pushed the fix/shellcheck-warnings branch from 0cf298b to c9398e7 Compare March 20, 2025 09:19
@csicar

csicar commented Mar 20, 2025

Copy link
Copy Markdown
Contributor Author

We can consider adding shellcheck to our automation to avoid this being a recurring change.

The way the ./container/*.sh scripts work at the moment, shellcheck violations are required: docker run ... ${GPU_STRING} for example required that the GPU_STRING is expanded to work correctly. So we would need to ignore the shellcheck warning explicitly there. A Github Action is available, so integrating it should not be that hard

@csicar

csicar commented Mar 24, 2025

Copy link
Copy Markdown
Contributor Author

Before this get's out-of-date. I think it might make sense to merge this. Adding a CI-Stage can probably be a separate PR

@harryskim

Copy link
Copy Markdown
Contributor

Please target 0.2.0 release.

@harryskim
harryskim merged commit 4d32e24 into ai-dynamo:main Apr 10, 2025
kylehh pushed a commit to kylehh/dynamo that referenced this pull request Apr 11, 2025
kylehh pushed a commit to kylehh/dynamo that referenced this pull request Apr 11, 2025
Co-authored-by: Meenakshi Sharma <163925564+nvda-mesharma@users.noreply.github.com>
oandreeva-nv pushed a commit that referenced this pull request Apr 22, 2025
Co-authored-by: Meenakshi Sharma <163925564+nvda-mesharma@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external-contribution Pull request is from an external contributor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants