-
Notifications
You must be signed in to change notification settings - Fork 17
Small cleanup #85
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
Small cleanup #85
Conversation
6c77a91 to
e483b5b
Compare
…hanged files since it can be too large for Bash to interpolate
…anism to iterate over all of them
…anism to iterate over all of them
…chanism to iterate over all of them
punkrokk
left a comment
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.
@blag Can we see this running somewhere? Everything makes sense, but would be cool to see if work.
cognifloyd
left a comment
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.
From reading the code, looks good.
nmaludy
left a comment
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.
LGTM!
|
@punkrokk Good point. FWIW, I did check this manually by running the CircleCI job for StackStorm-Exchange/stackstorm-aws#89 manually with SSH. However, I also opened StackStorm-Exchange/stackstorm-aws#100 to kick off CI with this. That PR is tiny, and it only triggers one part of this code: That verifies that it is properly writing and then reading from the I haven't tested the similar changes for JSON so please 👀 at that code closely, but the YAML and Python checks were run during my manual CI run for StackStorm-Exchange/stackstorm-aws#89. |
Kami
left a comment
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.
LGTM assuming existing jobs for various packs pass :)
|
Once I merge this I'll need to rerun CI for StackStorm-Exchange/stackstorm-aws#89 anyway (although that's exactly what I did manually), and that definitely exercises more of the changes here, so I'll know very quickly if I messed anything up. |
|
This code looks good. Failed test run: https://circleci.com/gh/StackStorm-Exchange/stackstorm-aws/626 (Python 2) |
As well as a few small cleanups, this PR also fixes the CI scripts and the
Makefileto work on PRs that have very large numbers of changed files, like StackStorm-Exchange/stackstorm-aws#89.This PR also fixes CI for this repository.
Related: #76