Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow dev to run alkiln in the playground or something similar #661

Closed
2 tasks
plocket opened this issue Mar 10, 2023 · 11 comments
Closed
2 tasks

Allow dev to run alkiln in the playground or something similar #661

plocket opened this issue Mar 10, 2023 · 11 comments
Assignees

Comments

@plocket
Copy link
Collaborator

plocket commented Mar 10, 2023

Right now through an interview.

Will add more, but quick note about sandbox: https://github.com/puppeteer/puppeteer/blob/main/docs/troubleshooting.md#setting-up-chrome-linux-sandbox

[Edit:

  • Complete MVP items
  • Write issues for all items in new repo

See list of items to complete below
]

@BryceStevenWilley
Copy link
Collaborator

BryceStevenWilley commented Mar 10, 2023

We have a proof of concept! Kiln and puppeteer can run on docassemble, with few caveats that we need to fix:

  • as mentioned above, we are running puppeteer with no sandbox. IMO, this is not as important, because we are only visiting the server that the code is running on; if you don't trust anything that you could download from your server, I've got bad news for you; it's already on your server. Eventually we can look at puppeteer's suggestions for sandboxing on linux, the setuid approach might work on docket with some extra setup steps, i.e. login to your docker container and run these commands. Some more sources on how to get things working, all pretty involved: https://stackoverflow.com/questions/62345581/node-js-puppeteer-on-docker-no-usable-sandbox
  • we need to add back the env variable for playground_id to Kiln. Right now, we assume that we get that information with the API key, but if we run on the server itself, we don't need the API key. The python side (i.e. running kiln through the interview) should be able to get us the user id (user_info().id), we just need to be able to pass that info back to Kiln again. This info was previously removed in Use api to get Playground ID instead of ID env var. #392. #513, so we should be able to add it back by looking at those changes.
  • getting everything to run through python in the background task. We have it all in bash, so things should be good there
  • undoing the hacky-ness that we've had to add in the branch for this
    • be able to pass in the paths that we want to search for *.feature files through the command line. docassemble stores the source files in such a strange location that the current glob can only find it in downloaded or installed projects, not in a playground project.
    • making sure that the sandbox option can also be configured; we do not want to start running without a sandbox on GitHub Actions

@plocket
Copy link
Collaborator Author

plocket commented Mar 28, 2023

A better articulation of one concern just occurred to me - we're installing npm packages on their server and that's the part that makes me wonder how developers will feel about that. [Maybe we can point them to the lines in the code that install alkiln and (after we shrinkwrap) tell them they can stick to a specific version of alkiln if they want to be extra careful. Would add it as an issue for the documentation about security.]

@plocket
Copy link
Collaborator Author

plocket commented Mar 28, 2023

Also, here are my notes from the duplicate issue I made about making ALKilnInThePlayground (to be) interview for devs who want to run the tests on their server:

The user would install the package on their server and thereafter have access to an interview they could run. At MVP, the interview would allow them to install an available version of ALKiln, pick tags for running the tests, and get the output of the tests.

The package needs a better name, but is already being worked on at https://github.com/plocket/docassemble-ALKilnInAPlayground. The finished functionality might be at MVP now. I think the major items missing, if we want to include them in MVP are:

  • Allow dev to uninstall ALKiln
  • Allow setting other env vars, like secrets (e.g. email/password for a log in step)
  • Detect badly installed alkiln and deal with it (even if it happened in some other interview)
  • Maybe when any install error, delete alkiln and re-install?

The functionality are MVP includes these:

  • Remove chaining for old node version (foo?.bar)
  • Early stop for ALKiln install with .revoke() and .failed()
  • Add metadata block
  • Require user to be logged in before running interview
  • Remove extra 'tmp' dir in unzipped zip
  • Put report at top of summary files
  • Use background action args
  • Don't install if new version matches the old version. Just exclude it from the list of options? No, that'll be more confusing if they can't find what they're looking for.
  • Delete unclosed... puppeteer browser folders? Files? puppeteer_dev_chrome_profile-zzzzz
  • Choose version of ALKiln with cached version
  • Show ALKiln version to user
  • Clean up files no matter what
  • Allow user to stop in the middle of the test and jump straight to output. See new revoke
  • [auto deleting now] Bug: deleting goes back to output page before going to end
  • start new session/interview
  • Style results page. Scenario background color?
  • Delete /tmp files after copying into da files (doesn't keep the folder structure, but I guess the zip does)
  • Use dynamic server url
  • Use dynamic project
  • Add project name env var to alkiln cli version.
  • Use dynamic user id
  • Use dynamic tags

Things that I think would be nice, but possibly not MVP:

  • Convert the python code to a module? Can we avoid this? Nicer for us to not wait for reload with every tweak.
  • Tell user if the version they have installed is the latest version?
  • Add "restart this session"? So they don't have to make a new interview for every test if they don't want to.
  • Coloring output: Cucumber formatter for some of it maybe https://github.com/cucumber/cucumber-js/blob/main/docs/formatters.md
  • Set other env vars (like max time per step)
  • Allow testing default project
  • In the spirit of the above item, handle with a situation where no Projects exist.

Future features:

  • Show the user a live update of the console as the tests are being run. This would include output from tests that were stopped early. Right now, the suggestion is to create a Flask app for that - a huge change. Possibly save cucumber output in a file in ALKiln itself and then read off that file every 10 seconds? Would need threading and I'm not sure how that would work. Maybe that could be a pipe...?
  • Offer a list of all available tags?
  • Rerun tests in the same interview? (without starting a new interview)

@plocket
Copy link
Collaborator Author

plocket commented Mar 28, 2023

Also, I'm not clear on what this means:

making sure that the sandbox option can also be configured; we do not want to start running without a sandbox on GitHub Actions

Does this refer to the puppeteer sandbox? Or another sandbox?

@plocket
Copy link
Collaborator Author

plocket commented Mar 28, 2023

Non-MVP item 1:

  • Set other env vars (like max time per step)

Discussion comments:

If max time per step is really the only other env var, if the server is reloading, the tests won't be able to detect it - they'll be stuck too.

Is that the only other env var? Also, might need to test what happens when the server is reloading.

Non-MVP item 2:

  • Allow testing default project

Discussion comments:

Not MVP, but would be nice as some new developers don't differentiate between Projects and the default Playground.

Response:
I'm not sure how likely it is that devs will be using ALKiln before they've even gotten a handle on Projects. Maybe if they have experience, but do some quick testing locally? I'm not sure what would happen if they have no Projects. Would that make the question with the Project options error?

@BryceStevenWilley
Copy link
Collaborator

we're installing npm packages on their server and that's the part that makes me wonder how developers will feel about that.

IMO, the main purpose of this package is to run npm packages, and that's fairly clear in the naming / functionality. I agree it's good to mention this in the security section, but like you mentioned, there are a lot of other things we can add first, like shrinkwrap, etc.

Set other env vars (like max time per step)

We talked about this on Teams, but the only time related env vars are the reload timeout seconds (defaults to 30 seconds) and setup seconds (here's a quick search to confirm, but you can also just skim session_vars.js). Setup seconds explicitly won't be used, and reload timeout is IMO not as useful, at the very least not MVP.

making sure that the sandbox option can also be configured; we do not want to start running without a sandbox on GitHub Actions

To get Puppeteer to run we had to turn off the sandbox that uses by default. I don't think this is a good idea to leave on by default; we should make it controlable by a env var until we are able to document a better (and certainly more complicated) way of running Puppeteer in the sandbox (see the first bullet of the comment you quoted).

I'm not sure how likely it is that devs will be using ALKiln before they've even gotten a handle on Projects.

I looks like you understood that as "devs don't know what projects are and only use the default project", and my intention when I said that was "devs don't know there's any difference between the default project and other projects. It's a weird UX for docassemble to treat them pretty much exactly the same, and then for us to say 'we don't work with the default one'". There is a technical reason behind it, sure, and that's why I don't think it needs to be MVP, but it is going to be confusing, even if documented.

@nonprofittechy
Copy link
Member

I looks like you understood that as "devs don't know what projects are and only use the default project", and my intention when I said that was "devs don't know there's any difference between the default project and other projects. It's a weird UX for docassemble to treat them pretty much exactly the same, and then for us to say 'we don't work with the default one'". There is a technical reason behind it, sure, and that's why I don't think it needs to be MVP, but it is going to be confusing, even if documented.

As one point of interest, I train all of my students to use projects right away (and not to use the default playground for anything). But it's not the most discoverable feature, so someone who learned Docassemble on their own might not know projects exist. I agree the different between the two is a bit abstract, but I wouldn't suggest prioritizing support of the default playground. Mostly because it's something people can get around with a simple extra step and they're already going to be memorizing arbitrary new things to do.

@plocket plocket self-assigned this Mar 28, 2023
@plocket
Copy link
Collaborator Author

plocket commented Mar 31, 2023

I think we need to actually spell out/document somewhere our rationale for using -g. I personally keep forgetting the details.

@BryceStevenWilley
Copy link
Collaborator

think we need to actually spell out/document somewhere our rationale for using -g. I personally keep forgetting the details.

The best comment about this I found in #614 (comment); IMO the reasons we moved towards -g were:

  • because semantically, Kiln is a command line program, not a library that needs to be added as a dependency of other npm packages
  • to let us run Kiln on any files, not just inside of an npm project (that we were having to "make" each run)
  • not as directly related, but the above two points let us move towards shrinkwrap as a way of actually freezing our dependencies instead of package lock, which had a lot of issues when installing Kiln without -g

plocket added a commit to SuffolkLITLab/docassemble-ALKilnInThePlayground that referenced this issue Mar 31, 2023
plocket added a commit that referenced this issue Apr 6, 2023
* Change cli, session_vars behavior for Playground testing

* Try to account for source files that are stored on S3
@plocket
Copy link
Collaborator Author

plocket commented Apr 9, 2023

Just occurred to me that max time per step could also be used for when the test is downloading big files or when it takes a long time to get to the next page. I think they can handle this in a couple ways, though. First, they can give a custom wait time in individual tests. Annoying if you need to do it every time, but still. Second, since devs will now be able to set arbitrary variables, though, they can handle this manually. It might be useful to add that to their config when they first install ALKiln. I'm not sure if it would be necessary to help them update it in the interview - that may complicate the interview unnecessarily.

@plocket
Copy link
Collaborator Author

plocket commented Apr 12, 2023

The lib has been released and these items copied to SuffolkLITLab/docassemble-ALKilnInThePlayground#8.

@plocket plocket closed this as completed Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants