-
Notifications
You must be signed in to change notification settings - Fork 570
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
Don't Repeat Yourself #6
Comments
You can also move all of the functions to this
And then instead of the line I have above you would simply add a dot in front of it:
|
Hey @johnwyles yes I agree there is room for improvement here. These scripts have been developed over a span of several years and some of them follow different patterns and could use some cleanup. I considered moving to a single script that handled all the common functions, but my concern is if someone is only interested in one tool and they wanted to run just that without cloning the whole repo and it is missing some function from another script. I'll have to review all the scripts and see which ones need updating then come up with a good way to support this without breaking anything in the process! |
@swoodford it's been nearly a year now - I am wondering if you should close this or if I can help address anything further as I have now some bandwidth opening up to assist |
it should be interesting to document dependencies. I for instance use a single script, downloaded from a beanstalk ec2 instance. If it had dependencies, just downloading this single script wouldn't be enough. Either document dependencies or clearly specify that there are indeed dependencies within the repository, so we are aware of that and instead of downloading single files, we clone the entire repo. |
Is your feature request related to a problem? Please describe.
Repeating yourself
Describe the solution you'd like
Pull the configuration into a single file
Describe alternatives you've considered
N/A
Additional context
N/A
You have this atop all of your scripts and if this ever changes, for whatever reason (which it probably won't), you'll be copy/pasting it all over again throughout. Also if someone wants to add some initialization steps beyond what is specified here you may benefit them as they could simply add to this included file. So instead of this:
Move it to
_pre_command.sh
and improve it:Then in all of your scripts simply put:
Or something more fancy (but that I think isn't really necessary):
And then you can put this in an
install.sh
script:Then you can tell users "To simply install these scripts run the following:
sh -c "$(curl -fsSL https://raw.githubusercontent.com/swoodford/aws/master/install.sh)" > /dev/null
"The text was updated successfully, but these errors were encountered: