Closed Carbrex closed 8 months ago
Also why doesn't these tests show up on my fork like how the lila's do.
I believe you can enable them for your fork if you visit https://github.com/Carbrex/lila-docker/actions as the repo owner
Looks like the docker build is failing because of an issue with the mobile repo. Could be temporary or maybe the dependencies or setup changed. I'll investigate.
Testing it out now. This is great!
Feel free to push as many followup commits as you want. I'll squash merge once it's ready.
I think when a PR is passed, we can skip the other prompts (optional services, passwords, etc) and go for an opinionated setup so that things just boot right away. In the future, we could control the optional services using the same technique if they're needed. Doesn't have to be a part of this PR, but I might try to do that if you don't want to.
So, do I not add any optional services, when the lila_pr variable is passed?
Thanks for fixing the failed tests thing
Should Cargo.lock be in .gitignore?
EDIT: nvm, I think it should not be in .gitignore
main.rs is now too big to be in a single file?
Should Cargo.lock be in .gitignore? EDIT: nvm, I think it should not be in .gitignore
Yes, you can commit it.
*EDIT: my mistake, I meant commit the lock file
main.rs is now too big to be in a single file?
I think it's OK. We'll ask @kraktus for a Rust review once it's ready, to be sure.
Looks like the functionality broke after these changes. If not for squash merge the commit history was really going to get polluted
Thanks for fixing it. I got it working on my patch-3 branch though. https://github.com/Carbrex/lila-docker/tree/patch-3 A separate lila_pr variable would help in making the other changes you mentioned above?
I think when a PR is passed, we can skip the other prompts (optional services, passwords, etc) and go for an opinionated setup so that things just boot right away. In the future, we could control the optional services using the same technique if they're needed. Doesn't have to be a part of this PR, but I might try to do that if you don't want to.
Yep good point, how about we add both to the struct. keep workspace_context
and add lila_pr_no
as its own.
Didn't mean to step on your toes with the changes. I had some in-progress edits while reviewing when you had pushed. I'll pause on making any changes until you've finalized.
Yep good point, how about we add both to the struct. keep
workspace_context
and addlila_pr_no
as its own.Didn't mean to step on your toes with the changes. I had some in-progress edits while reviewing when you had pushed. I'll pause on making any changes until you've finalized.
Ok, will complete within 10 minutes and then you can push your changes. Thanks for helping.
I have done my changes in patch-3 but I am having trouble merging my changes with yours. https://github.com/Carbrex/lila-docker/pull/1/files
Finally done. You can proceed now. Sorry for all this clutter.
tl;dr - Optional URL key/value will start a Gitpod workspace with a PR checked out instead of master.
Example: https://gitpod.io/new/#LILA_PR=14738/https://github.com/Carbrex/lila-docker/tree/patch-4
Those key/value pairs get automatically added to GITPOD_WORKSPACE_CONTEXT env so this PR parses and then runs the necessary git commands.
I’ll check tomorrow 👍
Nice work, only changed a few nits. The failure of test is not due to change, the tests are inherently flickered because of setting env variables for their own process.
Great feature. Thanks @Carbrex! :tada: :rocket:
Thanks for the review @kraktus!
Closes #17 Try this https://gitpod.io/new/#LILA_PR=14733/https://github.com/Carbrex/lila-docker/tree/patch-4
Works well on my local and also on gitpod. Any idea why the docker compose build test failed. Also why doesn't these tests show up on my fork like how the lila's do.