Skip to content
This repository has been archived by the owner on May 3, 2020. It is now read-only.

Safe init: do not overwrite cert, key and config if present, close #400 #413

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

schrnz
Copy link

@schrnz schrnz commented Feb 6, 2018

Essentially, I just check whether the files exist. Concerning the SSL key and cert, I regenerate them if at least one is missing.

puts "Copying configuration settings over."
File.open("./config.json", "w") do |f|
f.write File.open("./config.json.defaults", "rb").read
if !File.exist?('cert.pem')
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be checking if config.json exists, correct?

Copy link
Author

Choose a reason for hiding this comment

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

Ehh yes, sorry about that.

@schrnz
Copy link
Author

schrnz commented Feb 7, 2018

Thanks for pointing out the copy-paste error. I would suggest I fix this and force-push the branch so that it is only 1 commit (no pollution of commits after merge) and we do not have to create a new pr (would be kind of weird). I was told that force-pushing to pr branches is generally accepted, what is your opinion on this?

@BuffaloWill
Copy link
Contributor

I have no preference. I am good with you updating this PR or creating a new one.

@BuffaloWill
Copy link
Contributor

BuffaloWill commented Feb 8, 2018

I've realized we also need a check to see if the config.json is unchanged from the git repo, if so overwrite it. Otherwise, new installs won't get the newest features from config.json.defaults.

@schrnz
Copy link
Author

schrnz commented Feb 10, 2018

Updated the PR with the fixed version.

Concerning updates: If we assume config.json stays under versioning (see #407 ), then it should be identical to config.json.defaults, right? This implies that whenever someone is checking out Serpico, they get the most recent configs from the repo itself, so the first_time.rb script skipping the copy step does not make a difference...

Talking about this, I just realized I did not know that the config is under versioning when I created this PR. It effectively defeats the whole purpose of this PR since the original idea was having a docker environment where you could have your config included via volume mount, which (I just realized) will still be overwritten by the git checkout even if this PR prevent the first_time.rb script to do it.

I mean the PR is still valid as the current implementation is inconsistent, and it solves the problem for the key and cert as well. However, the config problem is still there... But I guess we should discuss this in another issue/thread.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants