Cleanup, more CI testing for scripts, validate inputs#49
Cleanup, more CI testing for scripts, validate inputs#49fingolfin merged 3 commits intogap-actions:mainfrom
Conversation
| if not IsBound(GAPInfo.PackageInfoCurrent) then | ||
| Print("Reading PackageInfo.g failed\n"); | ||
| FORCE_QUIT_GAP(2); | ||
| Exec("echo \"::error::Reading PackageInfo.g failed\""); |
There was a problem hiding this comment.
This file is a GAP "script", I mean, we can easily treat it as script with GAP as execution environment, but in general I'm not a big fan of including Actions logic into GAP.
Writing an abstraction layer could be helpful.
@fingolfin What do you think?
There was a problem hiding this comment.
I guess we could have a file gh_actions.g which contains code that re-defines Error to do something else (e.g. call Exec like here), and then change e.g.
gap -A -q $GITHUB_ACTION_PATH/scripts/pkginfo_to_json.g
to
gap -A -q $GITHUB_ACTION_PATH/scripts/gh_actions.g $GITHUB_ACTION_PATH/scripts/pkginfo_to_json.g
But I am not sure it's worth the trouble? After all, you can run pkginfo_to_json.g locally even with that Exec for errors in it.
There was a problem hiding this comment.
I.e. I am not sure what another abstraction layer would gain us, what benefit it would bring? Its downside is the usual: it adds complexity and thus opportunity for new bugs, and makes debugging harder.
(I am not saying there are no issues, just that I don't see them; happy to learn)
Co-authored-by: Kamil Zabielski <50334623+limakzi@users.noreply.github.com>
| if ! ./check_date.sh "$(date +%Y-%m-%d)" \ | ||
| || ! ./check_date.sh "$(date -d "1 day ago" +%Y-%m-%d)" \ | ||
| || ! ./check_date.sh "$(date -d "1 day" +%Y-%m-%d)" \ | ||
| || ./check_date.sh "$(date -d "2 days ago" +%Y-%m-%d)" \ | ||
| || ./check_date.sh "$(date -d "2 days" +%Y-%m-%d)" | ||
| then | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Doesn't GitHub run scripts with set -e anyway? Then we could just do this, and get better info about which check failed:
| if ! ./check_date.sh "$(date +%Y-%m-%d)" \ | |
| || ! ./check_date.sh "$(date -d "1 day ago" +%Y-%m-%d)" \ | |
| || ! ./check_date.sh "$(date -d "1 day" +%Y-%m-%d)" \ | |
| || ./check_date.sh "$(date -d "2 days ago" +%Y-%m-%d)" \ | |
| || ./check_date.sh "$(date -d "2 days" +%Y-%m-%d)" | |
| then | |
| exit 1 | |
| fi | |
| ! ./check_date.sh "$(date +%Y-%m-%d)" | |
| ! ./check_date.sh "$(date -d "1 day ago" +%Y-%m-%d)" | |
| ! ./check_date.sh "$(date -d "1 day" +%Y-%m-%d)" | |
| ./check_date.sh "$(date -d "2 days ago" +%Y-%m-%d)" | |
| ./check_date.sh "$(date -d "2 days" +%Y-%m-%d)" |
There was a problem hiding this comment.
@fingolfin I prefer your syntax.
I think GitHub does, but its for testing, I suppose.
fingolfin
left a comment
There was a problem hiding this comment.
I am in favor of merging this (and tagging it so it gets use) rather sooner than later.
ErrorbyExec("echo \"::error:: [...] \""); FORCE_QUIT_GAP(1);to make the errors more visible in GitHub Action logs.