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

install.sh uses ancient talisman release because newer binaries are incompatible with script. #247

Closed
derwent-m opened this issue Sep 8, 2020 · 6 comments

Comments

@derwent-m
Copy link
Contributor

Is your feature request related to a problem? Please describe.
When using install.sh, it installs talisman v0.3.2, which is over 2 years old. I tried replacing this binary with the latest, release, however when I tried triggering the hook, talisman responded with the help message and didn't actually check the commit.

git push
  -c, --checksum string          checksum calculator calculates checksum and suggests .talismanrc format
  -d, --debug                    enable debug mode (warning: very verbose)
  -g, --githook string           either pre-push or pre-commit (default "pre-push")
  -i, --interactive              interactively update talismanrc (only makes sense with -g/--githook)
  -p, --pattern string           pattern (glob-like) of files to scan (ignores githooks)
  -r, --reportdirectory string   directory where the scan reports will be stored
  -s, --scan                     scanner scans the git commit history for potential secrets
  -w, --scanWithHtml             generate html report (**Make sure you have installed talisman_html_report to use this, as mentioned in Readme**)
  -v, --version                  show current version of talisman
Enumerating objects: 10, done.
Counting objects: 100% (10/10), done.
Delta compression using up to 12 threads
Compressing objects: 100% (7/7), done.
Writing objects: 100% (7/7), 2.08 KiB | 2.08 MiB/s, done.
Total 7 (delta 4), reused 0 (delta 0)
remote: Resolving deltas: 100% (4/4), completed with 3 local objects.
...

Describe the solution you'd like
the install script should be updated so that it downloads a more recent talisman binary to the git hooks directory, and the git hook should be configured in a way such that Talisman is called correctly.

My suggestion:

  • update VERSION to "v1.8.0"
  • update SHAs
  • download talisman_hook_script.bash along with talisman binary into .git/hooks/bin
  • .git/hook/$HOOK_NAME is now just a one-liner that sets TALISMAN_BINARY=".git/hooks/bin/talisman" and calls .git/hooks/bin/talisman_hook_script.bash

Describe alternatives you've considered
Instead of downloading talisman_hook_script.bash you could use heredoc that has the basic functionality needed from talisman_hook_script.bash, but this would mean maintaining 2 different scripts that provide the same functionality.

Additional context
PR incoming!

derwent-m pushed a commit to derwent-m/talisman that referenced this issue Sep 8, 2020
derwent-m pushed a commit to derwent-m/talisman that referenced this issue Sep 8, 2020
derwent-m pushed a commit to derwent-m/talisman that referenced this issue Sep 8, 2020
@dcRUSTy
Copy link
Collaborator

dcRUSTy commented Sep 8, 2020

https://github.com/thoughtworks/talisman#installation doesnot instructs using install.sh from root directory. Apparently https://github.com/thoughtworks/talisman/blob/master/install.sh is a leftover from the past

P.S. My bad missed https://github.com/thoughtworks/talisman#installation-to-a-single-project

derwent-m pushed a commit to derwent-m/talisman that referenced this issue Sep 8, 2020
svishwanath-tw pushed a commit that referenced this issue Sep 8, 2020
…249)

* [Resolves #170] Add pre-commit option in install.sh

* [Issue #247] update VERSION and SHAs to v1.8.0

* [Issue #247] download talisman_hook_script.bash along with talisman into .git/hooks/bin

* [Issue #247] 🚨 Fix shellcheck warnings

* [Issue #247] 📝 update doc, single project install

* 📝 fix inaccuracy in documentation

* 🐛 Handle bug introduced in #246

Co-authored-by: derwentx <derwentx@gmail.com>
@derwent-m
Copy link
Contributor Author

Apparently https://github.com/thoughtworks/talisman/blob/master/install.sh is a leftover from the past

Hmmm, well it looks like this is the only way to install Talisman to a single repo according to the instructions. If it's a leftover from the past, should the instructions reflect that? Is there an alternative way to install Talisman to a single project?

@derwent-m
Copy link
Contributor Author

Thanks for merging. install.sh in the repo is up to date, however the script hosted at https://thoughtworks.github.io/talisman/install.sh is not! Do you think it's an issue that this install script doesn't update automatically based on what's in master? If so, I can create a new issue. Any who I can get in contact with to update it? (I'm an Aussie TWer by the way 😄👋 ) Doesn't appear to be mentioned in https://github.com/thoughtworks/thoughtworks.github.io , it seems like that website just loads content from https://www.thoughtworks.com/open-source . I'll ask around.

@svishwanath-tw
Copy link
Collaborator

svishwanath-tw commented Sep 9, 2020

@derwent-m install.sh and the docs are served from gh-pages branch of this repo.
I know @harinee is working on improvements and present all the features talisman supports in an user friendly way.

@derwent-m
Copy link
Contributor Author

@derwent-m install.sh and the docs are served from gh-pages branch of this repo.
I know @harinee is working on improvements and present all the features talisman supports in an user friendly way.

Cheers @svishwanath-tw , I've opened a PR for updating the gh-pages branch

svishwanath-tw pushed a commit that referenced this issue Sep 11, 2020
* [Issue #247] 🔨 update install.sh to use latest version

* ♻️ further reduce complexity of install.sh

Authored-by: derwentx <derwentx@gmail.com>
svishwanath-tw pushed a commit that referenced this issue Sep 11, 2020
* [Issue #247] ♻️ remove dependency on talisman_hook_script
* ♻️ further reduce complexity of install.sh

Authored-by: derwentx <derwentx@gmail.com>
derwent-m pushed a commit to derwent-m/talisman that referenced this issue Sep 19, 2020
🐛  fix SC2076 quoted regex

[Issue thoughtworks#247] 🏁  download Windows binaries
derwent-m pushed a commit to derwent-m/talisman that referenced this issue Sep 19, 2020
@derwent-m
Copy link
Contributor Author

install script has been updated with latest SHAs and version (at the time). Opened a new issue to address this into the future.

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