-
Notifications
You must be signed in to change notification settings - Fork 15
Improve check-sample github workflow #287
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
Conversation
I think #275 (comment) and #281 and this PR are all related. I couldn't get #280 to pass checks when I was building it and I gave up because it wasn't clear to me what was going on which caused all those samples which used to be fine to suddenly be marked as failing. The last time that happened there was a bug in the cli so the compose file check failed. I think we have some new checks that actually run a deployment? Is that correct @jordanstephens? If so can we document how to prepare everything in a sample to pass the checks? (and/or add those thing to be automatically added to the checklist) |
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.
@commit111 @raphaeltm It looks like the underlying error is related to these lines in scripts/check-sample-files.sh
:
samples/scripts/check-sample-files.sh
Lines 21 to 23 in 71f8f2f
if ! defang compose config > /dev/null 2>/dev/null; then | |
echo " - [ ] ${dir}compose.yaml contains errors" | |
fi |
Let's update that script to give us more information about the errors it found, and also to note where the errors came from. Maybe something like this:
- if ! defang compose config > /dev/null 2>/dev/null; then
- echo " - [ ] ${dir}compose.yaml contains errors"
+ output=$(defang compose config 2>&1)
+ if [[ $? -ne 0 ]]; then
+ printf " - [ ] %scompose.yaml is not valid according to \`defang compose config\`:\n \`\`\`%s\`\`\`\n\n" "${dir}" "$output"
@raphaeltm we made a few changes in this PR to address issues:
|
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.
I think we should move the nodejs code in .github/workflows/check-sample.yml
into a new script file. Maybe something like scripts/check-sample.js
, but it's not strictly necessary.
Yup. That makes a lot of sense. Okay, I guess I might have been mixing up a few things. I'll poke around all the samples PRs and try to get a better sense of things. |
Adding try-catch to improve error messages given by check-sample workflow.