Make alkiln action work on pull requests

[plocket]

Pull requests' .ref property is something like refs/pull/50/merge. What we need is the branch that made the pull request. Not just that, when the PR is made from a fork we need the originating branch somehow.

That latter point might make it necessary to change da core so it can pull in PRs, I'm not sure. That would be something like git fetch origin pull/50/head:a_branch_name. https://stackoverflow.com/a/30584951/14144258.

Similar to #757.

[plocket]

From a conversation with @BryceStevenWilley

The definitive option is to use dainstall on the copy of the repo we get from the checkout action. Or I guess we could see what checkout does for forks and do the same in our action.
This looks like what we might need: https://github.com/actions/checkout/blob/61b9e3751b92087fd0b06925ba6dd6314e06f089/src/ref-helper.ts#L13

[For dainstall] You can dainstall to a playground package
dainstall --playground --project "myprojectname"
It's a much harder coupling between Kiln and dainstall
We'd need to nail down the arg passing to run-alkiln though, since we wouldn't be writing the runtime_config.json anymore
[Otherwise], how would the script know the project name that dainstall pushed to? [Maybe it could] just be a wrapper around it... [We'd] need to have our setup.js directly call dainstall

And we should figure out how we want to fix this (we don't have to implement the fix, but we should be certain it'll work), before the other PRs [we're working on right now. https://github.com//pull/755, https://github.com//pull/753, and https://github.com//pull/740]. How we do this will affect installing on the server, and depending on how we do it, might affect what command line args we want to pass around and how.

[plocket]

This is a blocker because we need to decide whether we're using dainstall. If so, it could be easy to implement this. If not, we'd have to figure out how to get the correct branch of the correct repo or how to get da to pull from a PR.

With dainstall, we'd need to create the project name in the action and pass it in, as well as handling regular installations with the Playground API. That is, we'd need to handle multiple cases for project names. Not super complicated, but not ideal.

On the other hand, with dainstall, we could basically get rid of setup completely (we think). Local tests just need the API key and a special command. Or we could use setup just to create the project name.

dainstall is like uploading a zip file.

What do we do about takedown?

[BryceStevenWilley]

Quick thoughts on takedown:

docassemblecli / dainstall doesn't have any equivalent for takedown, so we can't use that.

• if running in the GitHub Action (i.e. we explicitly ran dainstall ...), we can run takedown as normal; it'll only delete the project we created.
• In my experience running things locally, I just don't run takedown. So I think that just works?
• Similarly, we don't need to takedown in ALKilnInThePlayground

TL;DL: we just leave takedown as is, should be fine

[plocket]

Seems we may not be able to just use dainstall to install Playground Projects. Someone tried to do so, but it only worked on pre-existing Projects. We'd have to at least use the API to create the project.

Still, we'd be able to output the name of the Project from alkiln-setup and use dainstall to avoid one of the main pain points here - finding the repo/branch of the PR (which still wouldn't let us test the PR, just that branch).

[michaelhofrichter]

@plocket That's not the case. I did a PR for dainstall so it will automatically create the Playground Projects if they don't already exist. That was jhpyle/docassemblecli#5. I thought I had issues with it recently, but when I tested yesterday it was working fine.

[BryceStevenWilley]

Someone tried to do so, but it only worked on pre-existing Projects. We'd have to at least use the API to create the project.

I haven't been able to test since I saw that mentioned, but I routinely use dainstal to create new projects in the playground, and haven't run into any issues. Michael comment sniped me and said the same thing. I am willing to test that feature further, and I can put effort into ensuring it works upstream if we find any issues.

[plocket]

It sounds like we've landed on using dainstall. If so, I'm going to finish up the current dangling PRs with this issue in mind and then start work on this one. I may implement this while working on those as the wires seem pretty tangled.

[plocket]

This isn't a blocker, but jhpyle/docassemblecli#10 will help with local development when using the new script - dainstall needs to be able to ignore more of our files so that the codebase won't be too big to install on the playground. It does this by using the .gitignore to ignore more files. This isn't a problem with our GitHub tests because the repo already ignores those files. [If it doesn't get pulled in, we'll have to add instructions to the repo setup.]

[plocket]

For Future People: If we want to get rid of dainstall and still run tests on the right repo of a PR, we can probably get the PR repo name and then use checkout to go to that repo: https://github.com/orgs/community/discussions/26903.

- uses: actions/checkout@v2
  with:
    path: 'ActualRepoName'

This might still not be sufficient. We think PRs give a merged version of the PR branch (when there are no conflicts) and this method wouldn't test that. I personally haven't researched whether that's true.