-
Notifications
You must be signed in to change notification settings - Fork 2
Tidy up scripts #1
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
base: code
Are you sure you want to change the base?
Conversation
That is a local hack to use the local git repo on my local machine. I just pushed some updates to sync with what I had on my machine and remove the local hack. |
c21edc8 to
57be77d
Compare
|
Sorry. There were more local hacks and a bunch of files I didn't commit. I just did a forced update on code branch. I have verified it at least works on my machine in a fresh checkout. The diff branch is now also uptodate with en-wl/wordlist. See the updated README for how to rerun the scripts on existing commits. |
4607c4e to
fa72a8d
Compare
|
I got it to run with some minor changes. Does this do a large operation per each diff? It's been running on my somewhat decent laptop for quite a while now (for the last 111 commits). I'm not sure how well GitHub CI will take this. |
|
Thank you for continuing to work on this.
What do you mean by large operation? It should take a few minutes per diff. The results are cached though in the git repo itself so it won't do unnecessary work. |
kevina
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That you for keeping the changes minimal. There is just a few minor things.
| export DBNAME=scowl | ||
|
|
||
| export PGVER=11 | ||
| export PGBINDIR=/usr/lib/postgresql/11/bin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not hard code PGVER. It should take the PGVER from the environment and if it's not set to the system default, and if that fails, fall back to 11. The PGBINDIR should then use PGVER to find the correct path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what exactly is wanted here, deleting the PGVER line?
I simply bumped it to current release since 11 has been EOL for a while.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like:
PGVER="${PGVAR:-11}"
I am not set on defaulting to 11. It is just what I use and is known to work. My system is also older and doesn't have the latest version available. If we get this working on CI than I am okay with bumping this to the current version that is support by the CI environment as I will no longer need to run it on my system.
| @@ -1,13 +1,13 @@ | |||
| . ./psqldb-env.sh | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason bash is needed here and #!/bin/sh won't work? The same applies to the other shell scripts. I have always used sh <script> and have not run into issues. On my system at least sh alias to dash and not bash.
If bash is needed for specific features that's OK. But I rather stick to the standard shell script unless there is a compelling reason not to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar enough with what's pure sh implementation and what's dash/bash when I write shellcode, so I default to bash.
I don't mind switching it to sh, though every system, the CI included, will have bash available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dash is a bit faster than bash, but it’s not something I am set on. I always use #!/bin/sh unless I have a good reason not to.
If you want to take a stab at getting the CI integration working, then switching to bash is fine if that’s what you are more comfortable with. Otherwise, I would prefer to stick with sh, as that’s what I would use.
|
I should add the CI task does need to commit the changes when done, so there might need to be some precautions to parallel runs that can cause commit conflicts. What I would ultimately like to see is that when a P.R. is submitted some sort of link to the diff in this repo is added to the P.R. There also needs to be some sort way to map the P.R. (and other branches) into a parallel branch in the this repo. I have very little experience with setting something like this up (and at this point not even sure if it possible). If you are willing to try and make this work I can give you the necessary access to do so. |

This all executables as such and starts using shebangs.
There's changes and safeguards added while I was trying to run this.
Sort of WIP since I never got it to run the setup properly, I do not get what
"${HOME}/wordlist/v2-pub"is supposed to be.Please squash when merging.