Closed juampynr closed 2 years ago
@fjgarlin thoughts about this so far?
Code-wise, it all looks good (my knowledge of go
is limited but it's all very readable).
go
first locally, and then run it?-- starts reading the very first comment...
Users would simply download the executable and run it.
that would address it.I'll test it all locally, just wanted to give some quick feedback as I found it.
Code-wise, it all looks good (my knowledge of
go
is limited but it's all very readable).
- I'd also expect as part of this PR changes in the readme file explaining how to use the new tool.
- Looking at the code, do we need to compile the command with
go
first locally, and then run it?-- starts reading the very first comment...
1: yep! I just adjusted that 2: no, I added a GitHub action that should create a release with the resulted binary that should be ready to use.
This is so awesome, it opens the door to future improvements like rewriting bash scripts in go, choosing the PHP version (once we figure out images), etc. And most importantly, it lowers the entry barrier to this project.
A super small suggestion. When you run the command, after choosing the platform, it takes around 5~10 seconds until you get the first line of output. For a second I wasn't sure if my machine was hanging, so it might be good to put a Starting...
line so the user knows they'll need to wait a few seconds.
So something like:
? Select CI provider:
▸ Bitbucket
CircleCI
GitHub Actions
GitLab CI
Travis CI
✔ Bitbucket
Starting...
(5~10 seconds later)
output: + wget -O...
Could be Starting...
or This might take a few seconds...
or something like that.
You missed replacing the old command with the new one here: https://github.com/Lullabot/drupal9ci/blob/82-interactive-cli/README.md#github-actions
Will continue later.
Great work so far!! 💪
This is so awesome, it opens the door to future improvements like rewriting bash scripts in go, choosing the PHP version (once we figure out images), etc. And most importantly, it lowers the entry barrier to this project.
You did read my mind! That's what we could do once this pull request lands.
@fjgarlin this is ready for another look.
:exclamation: No coverage uploaded for pull request base (
master@519a879
). Click here to learn what that means. The diff coverage isn/a
.
@@ Coverage Diff @@
## master #83 +/- ##
=========================================
Coverage ? 40.54%
=========================================
Files ? 1
Lines ? 37
Branches ? 0
=========================================
Hits ? 15
Misses ? 21
Partials ? 1
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 519a879...6309765. Read the comment docs.
@fjgarlin I will merge this tomorrow. Let me know if you find any issues. We can work on further changes in new pull requests.
I was away from the computer yesterday and couldn't check the activity. I've been checking the code changes and everything looks really solid.
I think it's good to be merged. Approving the PR. Great work!
@fjgarlin I may have broken some of the providers by updating the Docker images. I am going to merge and then will start testing them one by one with a sample repository. Feel free to suggest whatever fixes you may consider.
Not a problem. As this is not a package that is updated on people's projects, but rather a "use once, then tweak in your own copy", we will not be breaking people's projects.
If new users find issues I'm sure they'll report them. But yeah, in any case, it's a good idea to give it a quick try by yourself and if I find time next week I can also do the same, but all of it as follow-ups.
For #82
This is raw and lacks error handling and best practices but already does something:
drupal9ci
executable.As I am writing this I think that I can simply run the script without having to download it since it is packaged within the executable. I will fix this in this pull request.
Once we have this in place we could: