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

change docker-startup.sh config file handling, config file path detection and offer save path in setup #457

Merged
merged 3 commits into from
Mar 16, 2018
Merged

Conversation

twhiston
Copy link
Contributor

moves config file to non ephemeral location.

I think this is the only change needed to close #448
I would say it is also an alternative to PR #397 as we defer cleaning to outside semaphore with this approach

@strangeman could you give this a review and sanity check in your environment?

Once this is merged we should update the wiki with details of the approach we take to cleanup (/tmp or cronjob to clean data folder)

moves config file to non ephemeral location
@strangeman
Copy link
Contributor

I'm not using Semaphore in Docker, I use it just as plain binary. So, it looks like addition to #397

@twhiston
Copy link
Contributor Author

Actually i would disagree as #397 changes the folders where data is written for inventories, but not repo's etc... actually all of that stuff is ephemeral as we can just clone the repo again (and also no one should ever make manual changes to that data), and because it cleans up after every job you loose your audit trail about the task inventory etc.....
So i would propose that I additionally add a ConfigPath var (in addition to tmp path), and write the config to this path during setup, and use it as the default place to look for config when no path is specified on the cli. What do you think?

@strangeman
Copy link
Contributor

Yeah, that will be good. One path for the ephemeral stuff, another one - for config.

@twhiston
Copy link
Contributor Author

i'll try to get this done tonight. Thanks!

@twhiston
Copy link
Contributor Author

twhiston commented Mar 6, 2018

This should also add an additional small feature during setup and startup

-Ask for a separate config output dir, offer the cwd as a default and if no -config option is provided look in the cwd for a file to use. It should write out the path of the config file used to startup during the initialisation procedure as a sanity check for users

@joernott joernott mentioned this pull request Mar 6, 2018
@gcavalcante8808
Copy link
Contributor

+1 On this. The changes can make the setup/bootstrap much simplier. Today, I use a bunch of sh files to 'mount' the options used in the wizard, https://github.com/gcavalcante8808/docker-semaphore/; with this, a simple config file will make the job clean.

@twhiston
Copy link
Contributor Author

twhiston commented Mar 6, 2018

@gcavalcante8808 my comment above was more about subsequent runs. ie with -setup you have to do the interactive setup (or use the docker bootstrapping script stdin trick) and then in subsequent runs it will either take the -config flag for a fixed location or look in the cwd if none is provided.

I assume from your comment that you would also like to provide a config file, or something, when doing the initial setup, is this correct? If so how do you think we should approach this? Maybe if we run setup but with the -config flag provided it skips all the questions and moves on to the part that bootstraps the db directly, would that solve your issues? Would it also be useful to try to divine these settings from env vars?
If so I could see a strong advantage to moving the whole cli part out to use Cobra/Viper, make setup a totally seperate command and give the ability to pass in options for all the values, plus the automatic env var merging that viper can do, this would make it extremely flexible but its a more major update as it's technically a breaking change that would require a major version bump

@gcavalcante8808
Copy link
Contributor

gcavalcante8808 commented Mar 6, 2018

@twhiston "I assume from your comment that you would also like to provide a config file, or something, when doing the initial setup, is this correct?" In truth, I do the stdin 'trick' to provide all options because the migrations are applied when the setup runs, at least in the version 2.4.1.

" Maybe if we run setup but with the -config flag provided it skips all the questions and moves on to the part that bootstraps the db directly, would that solve your issues?":

Normally I like to provide an --migrate option that is idempotent and delegate the control of the db updates to the user.

@twhiston
Copy link
Contributor Author

twhiston commented Mar 6, 2018

well we have that option already as if you pass --migrate the program will perform the updates and then exit (see the ci config for a place this is used), but of course it will also always try to run migrations on every startup with the current config. The only thing this will not do is create the user in the database, because this logic is part of the --setup option.

So just to try to bring this to an actionable task, what do you need, apart from (I have rephrased this slightly for clarity) for your use case:

"When running -setup ask for a separate config file output dir, offering the cwd as a default.
When running Semaphore without the -setup option and no -config option is provided look in the cwd for a file to use. Semaphore should log (write out the path of) the config file used to startup during the initialisation procedure as a sanity check for users"

@twhiston
Copy link
Contributor Author

merged develop in to this branch and made the changes discussed and some tidying:

  • Adds setup question asking where to output config. Defaults to cwd, and if getting cwd produces an error default falls back to /etc/semaphore
  • Make docker autostart use env var or default /etc/semaphore for config file storage question
  • rename semaphore_config.json to config.json (maybe this is controversial but i don't see the need for the long name, and it should be non breaking since if you specify the config explicitly, as we all do now, it will still work as it always did)
  • If no config is specified try to use cwd/config.json
  • Show default value for playbook path in setup questions
  • Show actual config file used when starting semaphore

Adds setup question asking where to output config. Defaults to cwd, and if getting cwd produces an error falls back to /etc/semaphore
Make docker autostart default for config
Show default for playbook path
rename semaphore_config.json to config.json
fix pathname for circle
@twhiston twhiston changed the title change docker-startup.sh config file handling change docker-startup.sh config file handling, config file path detection and offer save path in setup Mar 14, 2018
Copy link
Contributor

@strangeman strangeman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@twhiston twhiston merged commit caec53e into semaphoreui:develop Mar 16, 2018
@twhiston twhiston deleted the startup_script_config_path branch March 16, 2018 08:59
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.

Semaphore don't delete the inventory files
3 participants