Regenerate index.js with GitHub actions #334
Conversation
rotu
force-pushed
the
ci-build
branch
4 times, most recently
from
2eef133 to
49cc5a5
Compare
|
This is really interesting - thanks for exploring it. This approach could be generically useful across all typescript Action implementations. I wonder if instead of trying to commit on the development branch, we could have an Workflow that is triggered on push to the default branch, that builds and writes to a release branch, which is where the marketplace actually reads from. This way instead of modifying PRs, we can just run tests against PRs, then have the release artifact created after merge (and never checked into the development branch) |
You betcha!
I assumed that checking it in in the development branch was a desirable feature so I didn't question this arrangement. Let me give it a whack... |
rotu
force-pushed
the
ci-build
branch
2 times, most recently
from
ed5abd8 to
f174716
Compare
|
@emersonknapp Done. |
rotu
force-pushed
the
ci-build
branch
3 times, most recently
from
49b9608 to
0136f93
Compare
rotu
requested review from
emersonknapp and
dabonnie
and removed request for
a team
|
I like that putting it on a different git ref means you don't have to merge with every push. I don't like that merging a PR isn't the full picture (and that to make this work, I'll have to modify existing tests to not simply use @emersonknapp, how do you see this working and how would you like to move forward? |
| @@ -80,7 +80,7 @@ export async function execBashCommand( | |||
| ): Promise<number> { | |||
| commandPrefix = commandPrefix || ""; | |||
| const bashScript = `${commandPrefix}${commandLine}`; | |||
| const message = log_message || `Invoking "bash -c '${bashScript}'`; | |||
| const message = log_message || `Invoking "bash -c '${bashScript}'`; | |||
There was a problem hiding this comment.
extra spaces in this file intentional?
There was a problem hiding this comment.
ha. No - I was only adding whitespace to this file to test the rebuild hook.
| @@ -0,0 +1,23 @@ | |||
| name: "Build" | |||
| on: push | |||
There was a problem hiding this comment.
limit to branches: [master]
There was a problem hiding this comment.
I think I'm not on the same page as you here. The current behavior is to build in a precommit step so that all branches have an up-to-date dist/index.js. Why should we only build some branches?
| name: "Build" | ||
| on: push | ||
| jobs: | ||
| test_ros: |
There was a problem hiding this comment.
name appropriate? maybe build_and_commit_to_release
.github/workflows/build.yml
Outdated
| git config user.name "ROS Tooling [bot]" | ||
| git add --force dist | ||
| git commit --no-verify -m "$commitmsg" --only dist | ||
| git push origin --no-verify HEAD:refs/tags/build/${{github.sha}} |
There was a problem hiding this comment.
I was thinking we would push to a release branch. It would need a few additional files (action.yml, README, LICENSE) - I think the best way would be to have it be a pure-augmentation of the master branch. So maybe it would look something like
# dist is never checked in on master
npm run build
git checkout release
git rebase master
git add dist/index.js
git commit -m "Build artifacts for ${{ github.sha }}"
git push origin release
We would then only create tags when we actually want to release out a new version of the action, which could be manual or on an otherwise automated schedule. If release naming is confusing then maybe we call the branch staging?
There was a problem hiding this comment.
It won't do to have a release branch.
- That doesn't solve the problem for building PRs.
- Workflow runs are not guaranteed to be sequential
We'd basically have to have a shadow build/xxx branch for every xxx branch and ensure that they run in order even when simultaneous commits are run.
rotu
force-pushed
the
ci-build
branch
5 times, most recently
from
c582150 to
710e47c
Compare
|
@emersonknapp I submitted a couple alternatives for how this could work here and none of them seem as straightforward as just committing on the dev branch: I think it might be more productive at this time for either you to take over dev on this or for us to hop on a phone call. I feel like I'm getting frustrated and going nowhere and you have a clearer vision of what would be an acceptable way to move forward here. |
Signed-off-by: Dan Rose <dan@digilabs.io>
|
Hey @rotu sorry for the slow response. I should have some more time to be able to think about this starting in a couple weeks, if all goes according to plan. I can try to take over on moving it forward then, in the meantime we should probably just continue on with checking in index.js with PRs. It's a bit of an awkward situation, I agree, but I want to make sure we don't do something too awkward as the cure, is all. |
|
@emersonknapp Thanks! I would love if you took it over - there are design choices here that I don't think I'm in a position to make |
I keep forgetting to build with PRs or opening PRs from the github web interface. Worse, index.js depends on the version of packages you have locally, which causes churn. This auto-generates the js file on each commit.