Skip to content
This repository was 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
schrnz wants to merge 1 commit intoBuffaloWill:masterfrom
schrnz:400_safe_init
Open

Safe init: do not overwrite cert, key and config if present, close #400#413
schrnz wants to merge 1 commit intoBuffaloWill:masterfrom
schrnz:400_safe_init

Conversation

@schrnz
Copy link
Copy Markdown

@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
Copy Markdown
Owner

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Owner

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

@BuffaloWill
Copy link
Copy Markdown
Owner

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.

…ffaloWill#400

If either key or cert is missing, both are regenerated
@schrnz
Copy link
Copy Markdown
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