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

WIP Let user set a custom path to openssl #111

Merged
merged 7 commits into from
Feb 27, 2021
Merged

Conversation

jmurty
Copy link
Collaborator

@jmurty jmurty commented Feb 12, 2021

This is work-in-progress. See #108

Add the optional --openssl-path argument when initialising a repo to
tell transcrypt to use an explicit path to the openssl binary,
instead of using whatever version is on the user's path.

The openssl path is now saved as a new transcrypt.openssl-path Git
config local setting in the repository, alongside the other transcrypt
settings like cipher, password etc.

If the user provides --openssl-path this explicit path is stored in
the local Git config, otherwise the default value 'openssl' is stored
which will fall back to the default behaviour of finding openssl on
the user's $PATH.

The --openssl-path argument gets special treatment for upgrades: an
--openssl-path argument value given along with --upgrade will replace
the existing config setting, despite config settings normally being
retained across upgrades.

Add the optional --openssl-path argument when initialising a repo to
tell transcrypt to use an explicit path to the openssl binary,
instead of using whatever version is on the user's path.

The openssl path is now saved as a new transcrypt.openssl-path Git
config local setting in the repository, alongside the other transcrypt
settings like cipher, password etc.

If the user provides --openssl-path this explicit path is stored in
the local Git config, otherwise the default value 'openssl' is stored
which will fall back to the default behaviour of finding openssl on
the user's $PATH.

The --openssl-path argument gets special treatment for upgrades: an
--openssl-path argument value given along with --upgrade will replace
the existing config setting, despite config settings normally being
retained across upgrades.
Copy link
Owner

@elasticdog elasticdog left a comment

Choose a reason for hiding this comment

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

This is looking nice! I like the solution of storing the path in the config and can appreciate the troubles that some homebrew setups can introduce. I left a couple of comments.

transcrypt Outdated
@@ -973,6 +985,8 @@ rekey=''
show_file=''
uninstall=''
upgrade=''
openssl_path='openssl'
openssl_path_given=''
Copy link
Owner

Choose a reason for hiding this comment

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

You could probably just use a single variable and check if it's empty before setting the default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I reconsidered and dropped support for changing just the openssl path when --upgrade-ing a repo, since it was weird to make that setting the one and only user-defined setting that could be changed when upgrading. Instead, the help text now says how to change it using git config

transcrypt Outdated
--openssl-path=PATH_TO_OPENSSL
use a specific openssl binary instead of the version in \$PATH.
Use this with --upgrade to update transcrypt's openssl path

Copy link
Owner

Choose a reason for hiding this comment

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

The README and man page will also need a refresh with the new option flag information.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have updated the README, changelog, and man pages. The help text itself could probably use some editing, it became log with the inclusion of the git config instruction

I reconsidered the feature that let users set or update a custom
openssl path using the --upgrade flag. This was unusual behaviour:
no other transcrypt settings could be altered during upgrade.

Since it is somewhat likely a user will need or want to change their
openssl path over a repository's lifetime, the help text now says
how to do that using a standard git config-setting command.
* `--openssl-path`=<path_to_openssl>:
use OpenSSL at this path; defaults to 'openssl' in $PATH.
To change after initialization:
git config transcrypt.openssl-path NEW_PATH
Copy link
Owner

Choose a reason for hiding this comment

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

Hmmm...it would be nice to not ever need to drop to raw git config commands at all. Would it make more sense to have this be named --set-openssl-path and allow it to be called whenever (during initialization or otherwise) to update the setting?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a really good idea. I have made this change, and it makes it much clearer when --set-openssl-path can be used.

Change the command to --set-openssl-path which applies the Git config
change regardless when the option is used:
- included with int
- included with upgrade
- by itself

This change greatly clarifies when the user can run the command
(whenever they like) and uncouples it from the init and upgrade
procedures.
Copy link
Owner

@elasticdog elasticdog left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for iterating on it!

# Via GitHub
* master:
  Install entire transcrypt script into repository
  Change version to indicate development "pre-release" status

# Conflicts:
#	transcrypt
@jmurty jmurty merged commit dce1ad0 into master Feb 27, 2021
@jmurty jmurty deleted the add-openssl-path-config branch February 27, 2021 11:03
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.

2 participants