@diosmosis opened this Pull Request on September 29th 2021 Member

Description:

I haven't used github actions before so this is mostly a guess. @sgiehl can you take a look and see if there's anything off? I'm also not entirely sure how to test this. Unless I can invoke the action in this PR w/o merging into 4.x-dev?

Review

@sgiehl commented on September 30th 2021 Member

It can only be executed/tested when it's in the base branch. But you could create a fork where you add the action in the base branch to test it on the fork.

@diosmosis commented on October 12th 2021 Member

@sgiehl / @tsteur this is ready for review. The action will now always run for every PR, build vue files, and if there are changes, will push to the PR so the files are always up to date.

@tsteur commented on October 13th 2021 Member

@diosmosis looks overall good to me. Might wait with merging until @sgiehl is back from holidays.

Sometimes there might be merge conflicts when different PR changed files in a same plugin etc. In that case we simply need to rebase 4.x-dev into the branch, possibly resolve merge conflicts manually (probably would need to call the build command ourselves) and commit again?

Not sure if I fully understand what the action does.

@diosmosis commented on October 13th 2021 Member

Not sure if I fully understand what the action does.

It just runs vue:build every time a PR is updated, and if there are changes, commits them (in case a developer forgets to re-compile).

Sometimes there might be merge conflicts when different PR changed files in a same plugin etc. In that case we simply need to rebase 4.x-dev into the branch, possibly resolve merge conflicts manually (probably would need to call the build command ourselves) and commit again?

Mostly, yes. The UMD files don't need to be looked at, other merge conflicts have to be handled, then either run build manually, or push and this command would build again. It doesn't solve the case where the merge is fine except for the UMD files which we just want to build again. Not sure there's a way around that.

@diosmosis commented on October 18th 2021 Member

@sgiehl updated the pr

@diosmosis commented on October 19th 2021 Member

@sgiehl ok, it's working now

@diosmosis commented on October 20th 2021 Member

@sgiehl left a reply above. @tsteur do you want to take a look at this as well?

@tsteur commented on October 20th 2021 Member

@diosmosis had a very quick look and looks all good to me. Looks good to merge to me and we can always change things as needed IMO

This Pull Request was closed on October 20th 2021
Powered by GitHub Issue Mirror