chore(common): unify pull-request creation scripts #9888
Conversation
github-actions
bot
added
web/
common/
common/resources/
User Test ResultsTest specification and instructions User tests are not required |
| git push origin "$branch" | ||
| builder_echo "Push complete" | ||
|
|
||
| hub pull-request -f --no-edit -l auto |
There was a problem hiding this comment.
It's "probably fine" for our CI agents, so long as we note the requirement... just noting that this assumes hub is available. Is that documented somewhere?
Asking because I don't have (or need) hub installed in my personal dev-environment, and I don't think it's listed in our dev prerequisite list. Not sure where we'd document extra resources needed for the agents.
There was a problem hiding this comment.
Yeah, given this is just for CI. Note that this is listed in the reqs in the comments for the script.
|
|
||
| builder_echo blue "Testing ci_open_pull_request" | ||
|
|
||
| ci_open_pull_request "$test_repo" "test/ci" "chore: testing CI" |
There was a problem hiding this comment.
So... does the test actually make a real pull request to the destination site (ci-test-repo) when run and leave it up? No cleanup for the results of this call?
Granted, I suppose that might be why ci-test-repo is there - to prevent pollution of our production repos with test PRs of this type?
There was a problem hiding this comment.
Yes. And yes. You can even look at said repo and see my test PRs (now manually cleaned up). Given the script is designed for manual run, I figured that was okay.
| # | ||
| # Prevents 'clear' on exit of mingw64 bash shell | ||
| # TODO: is this script still necessary or is /web/ci.sh a complete replacement? |
There was a problem hiding this comment.
This script was in its prior state due to the half-and-half state for the s.keyman.com build action. At the time, we didn't want the .sh script to actually commit the PR and changes... but prepping them was fine.
So, now that ./ci.sh prepare:s-keyman-com is no longer just preparing the files for the PR but also pushing the commit (and making the PR) itself... nah, should be fine to fully fold in the remaining bits into the ci.sh script. It was not a full replacement, but it seems your solution to #9886 is to make (or... "has made"?) ci.sh a full replacement.
Re: this question and the one from #9886 -
why is s_keyman_com.sh in web/src/tools/testing? Seems like the wrong place for this.
Ok, yeah, not sure why I ended up putting it there. Does seem like more of a 'building' one, but... oh well.
There was a problem hiding this comment.
I briefly looked on TC Web configs. Apparently the Release (push release to s.keyman.com) is still using Powershell. 😕
There was a problem hiding this comment.
s_keyman_com.sh seems redundant now -- it is never called afaict. Should we remove it?
There was a problem hiding this comment.
I've updated the TC Web configs to skip the git commit phase if .build-builder is present. We'll need to port this PR back to 16.0 as well, and then we can convert the entire script into a bash call.
|
Changes in this pull request will be available for download in Keyman version 17.0.202-alpha |
Fixes #9886.
Note: keyboards repo has a similar script, which we could unify in the future.
@keymanapp-test-bot skip