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

Go Rewrite #129

Closed
wants to merge 180 commits into from
Closed

Go Rewrite #129

wants to merge 180 commits into from

Conversation

gabyx
Copy link
Contributor

@gabyx gabyx commented Sep 30, 2020

Progress

A draft PR to have you guys in the boat.

The progress of porting this application could be like this:

  • Make a Go project
  • Rewrite base-template.sh in Go (leave cli.sh and install.sh in shell)
  • Make all existing bash tests (don't rewrite them) work and execute them with the base-template.sh GO executable.
  • Discuss deprecation: LINK
  • Clean up inital implementation.
  • Rewrite install.sh (leave cli in sh) in Go
    • When a update is pulled, we compile the project (builds base-template,cli, install) and dispatch to the compiled new install executable. The install then does the exact same as it does now.
    • Make all tests work with the new install executable.
  • Rewrite uninstall.sh in Go
    • Make all tests work with the new install executable.
  • Release setup with goreleaser.
    • Make download binary funcationality for github.com / and gitea download urls.
      The version corresponding to the determined tag in the release clone is downloaded and installed.
      Checksummed and signed with GPG.
  • Rewrite cli.sh in Go
    • Make all tests work with the new cli executable.
  • Refactor and cleanup, deprecate unused stuff.
  • Make the install/update procedure better and more concise (think it over more!...)
    • Write tests for it. Deprecate the once in so far...
  • Unix & Windows test pass.

@gabyx gabyx marked this pull request as draft September 30, 2020 21:23
@gabyx
Copy link
Contributor Author

gabyx commented Sep 30, 2020

Makeing good progress.
Keeping the implementation as close as possible because I think its pretty good as it is. Also to make the test work at the end...

Go is a charm sometimes a bit to easy, but good to keep it that way (miss generics...)
Its so much better to go with a proper language than Bash...

Some insights from the runner:
image

@coveralls
Copy link

coveralls commented Sep 30, 2020

Pull Request Test Coverage Report for Build 911

  • 5 of 6 (83.33%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 80.237%

Changes Missing Coverage Covered Lines Changed/Added Lines %
base-template.sh 3 4 75.0%
Totals Coverage Status
Change from base Build 907: 0.0%
Covered Lines: 2371
Relevant Lines: 2955

💛 - Coveralls

@gabyx gabyx changed the title Go refactoring Go rewrite Sep 30, 2020
@gabyx
Copy link
Contributor Author

gabyx commented Sep 30, 2020

image

@gabyx gabyx closed this Oct 1, 2020
@gabyx gabyx deleted the feature/go-refactoring branch October 1, 2020 17:25
@gabyx gabyx restored the feature/go-refactoring branch October 1, 2020 17:28
@gabyx gabyx reopened this Oct 1, 2020
@gabyx
Copy link
Contributor Author

gabyx commented Oct 6, 2020

@rycus86, @matthijskooijman Do you know what [ -x "$file"] does. I think its not the same as checking if the "owner" has execution right. I am not sure. Does it also incorporate if the user is in the group and the group has the executbale flag set?

@matthijskooijman
Copy link

Do you know what [ -x "$file"] does

man [ says:

  -x FILE
         FILE exists and execute (or search) permission is granted

I suspect this checks whether the current user has effective "x" permissions on the file (because they are the owner and the owner has +x, they are in the owning grou and the group has +x or others have +x)?

@gabyx
Copy link
Contributor Author

gabyx commented Oct 8, 2020

I found a solution: 53db07b#diff-a5982e9eccefb8a42e5403f0a6324886

I also directly added a new feature to define a runner cmd (first good solution, needs more thoughts) for hookPath by specifying the command in the file hookPath + ".runner if it exists.
GetHookRunCmd returns the run command cmd arg1, ... argN in case it is not executable, if its an executable (unix -> access C-function, windows: check ".exe" (first good solution)) it returns nil meaning we can directly execute the file.

Other hook manager configure everything in a yaml/json file (lefthook). The thing about that is, that you need to read the whole config in every hook run, which is a bit inefficient. So I am not starting to go down this path... I stay as closesly to the existing implementation, and make useful adjustments if it proves to be no problems for the tests.

I think first design principle is: Read as less config values and files as you can when executing the runner.
If you need to, only read it when you really need to. Currently the following is read always at startup:

  • ignore patterns (.ignore) (simple .yaml, not yet sure)
  • checksum file (simple .yaml, not yet sure )

Not yet sure: may be we can also leave the file format as it was, to be faster...

If you want more speed: -> Make a service which provide and cache these values for a repository (overkill)

@rycus86
Copy link
Owner

rycus86 commented Oct 9, 2020

I'd suggest adding some timing output for debugging, I don't necessarily think reading config should take that long to be honest, so perhaps don't worry about it for now (especially if you intend to keep the existing tests for the rewrite). I'd keep config and operations the same (just replaced implementation) then once it's ready to be switched over it'll be easier to change config formats and such.

@gabyx gabyx force-pushed the feature/go-refactoring branch 2 times, most recently from 06338da to c495637 Compare October 10, 2020 11:10
@gabyx
Copy link
Contributor Author

gabyx commented Oct 10, 2020

@matthijskooijman , @rycus86 : Do you know something about that?
https://stackoverflow.com/questions/64294818/how-to-get-controlling-terminal-cross-platform-in-go
There is also: https://github.com/mattn/go-isatty, but dont know if that helps...

@gabyx
Copy link
Contributor Author

gabyx commented Oct 10, 2020

@rycus86 If you wanna try the prompting some first steps: -> Readme

./build.sh && ./test.sh

@rycus86
Copy link
Owner

rycus86 commented Oct 11, 2020

@matthijskooijman , @rycus86 : Do you know something about that?
https://stackoverflow.com/questions/64294818/how-to-get-controlling-terminal-cross-platform-in-go
There is also: https://github.com/mattn/go-isatty, but dont know if that helps...

I fought with this a lot, even just targeting Linux, but it's a sh*tshow... 🙈
See https://github.com/rycus86/ddexec/blob/master/pkg/exec/streams.go -- though I never managed to get it working properly, I still have problems when you pipe input/output, etc.

What do you need this for here specifically?

@gabyx
Copy link
Contributor Author

gabyx commented Oct 11, 2020

Ah thanks for pointing me to: https://github.com/moby/term
I need a portable way of making a terminal. On Unix I open /dev/tty. on windows (good luck).
Maybe moby/term can make a terminal for us on any platform. Connecting to /dev/tty on Unix and to a new allocated terminal on windows. Dunno ;-) ->Solved...

@matthijskooijman
Copy link

@matthijskooijman , @rycus86 : Do you know something about that?

Nope, no experience with getting terminals on Go or cross-platform, sorry.

@gabyx gabyx force-pushed the feature/go-refactoring branch 3 times, most recently from b4e00ee to c088673 Compare October 15, 2020 21:18
gabyx and others added 22 commits January 21, 2021 19:15
-  No '--stdin' where no stdin attached. Under docker we have no stdin why
   it works. Under windows stdin is attached so it blocks.
    That was a replace mistake.
- Windows tests need a proper temp. dir. Therefore use
  everywhere a env variable.
- On windows we might need 'sh -c'. For now disable it.
- Cleaned runner executable: ${hookPath}  will be replaced.
- Better for args feed through
- Small output fix in runner
- Build own git-lfs executable.
  A shell script will not work on all platforms.
- golint corrections.
- Properly delete git-core/templates/hooks
  folder.
- Todo: This needs rework. See Readme.md
@gabyx
Copy link
Contributor Author

gabyx commented Jan 22, 2021

@rycus86 , @matthijskooijman :
Almost finished:
Check: https://github.com/gabyx/githooks

@rycus86
Copy link
Owner

rycus86 commented Jan 22, 2021

@rycus86 , @matthijskooijman :
Almost finished:
Check: https://github.com/gabyx/githooks

Nice one! I ought to check it out at some point, will try to find some time soon.

@gabyx
Copy link
Contributor Author

gabyx commented Jan 26, 2021

@rycus86:

I can finally close this. It was huge work, good fun, grulesome migration with the test (esp. windows...), but I think its in a good shape. There is stilll some stuff missing out for the various deploy scenarios.
Github works well so far as I am testing the updates at the moment. The release process is a breeece -> Just push a tag -> Github Action with Goreleaser will do the rest :-) 👍 Its signed too! Verified during download of course!

I added you as a collaborator (feel free =).

You should probably not push into master right now, rather open PRs if you want, I am still cleaning up a bit.
I will definitely protect "main"-branch, and each PR needs a review before we can merge it.

It would be cool if you could post a bold sign on the front page of the old implementation redirecting to the new one :-).
Maybe better when I am confident enough to release the beast ;-).

Ok sad that the Github stars do not get transferred ;-), but anyway, your vision and features are the right implementation I think for a hook manager, thats why I like it. Its powerful, but still fully configurable.

A review would have been nice, but jeah everybody has little time, so far I kept to my own principles -> clean and as proper as possible... maybe you could read through the Readme.md and questions, english corrections and suggestions may pop up, which I can still take up and implement, that would help :-)

-> https://github.com/gabyx/githooks

@gabyx gabyx closed this Jan 26, 2021
@gabyx
Copy link
Contributor Author

gabyx commented Jan 26, 2021

Ah of course: We need covarge, that another ticket I need to address...

@rycus86
Copy link
Owner

rycus86 commented Jan 26, 2021

Awesome news @gabyx well done!
I'll try to fix up the README here and open a PR on yours for spelling things today if I can get to it.

Thanks a lot for taking on the evolution of this project!

@gabyx gabyx deleted the feature/go-refactoring branch February 4, 2021 18:24
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

Successfully merging this pull request may close these issues.

None yet

4 participants