-
Notifications
You must be signed in to change notification settings - Fork 9
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
Fix installed plugins to use a dedicated dir to prevent naming conflict #347
Conversation
d8197f8
to
8719afc
Compare
8719afc
to
c32c755
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set to "request changes" to remove the schema config reference
Also, we'll probably want to check to be sure that all plugins that reference other plugins (helm references aws and kubectl, for example) are doing so via the $BITOPS_PLUGINS_DIR
env var)
Plugin confirmed usage of
Plugins confirmed not to use
|
scripts/plugins/deploy_plugins.py
Outdated
@@ -45,7 +48,7 @@ def deploy_plugins(): # pylint: disable=too-many-locals,too-many-branches,too-m | |||
|
|||
bitops_dir = "/opt/bitops" | |||
bitops_deployment_dir = "/opt/bitops_deployment/" | |||
bitops_plugins_dir = bitops_dir + "/scripts/plugins/" | |||
bitops_plugins_dir = BITOPS_plugin_dir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable is used only once. We could remove the unneeded variable assignment from this line:
bitops_plugins_dir = BITOPS_plugin_dir |
and use the original BITOPS_plugin_dir
var later below:
bitops/scripts/plugins/deploy_plugins.py
Line 113 in d71a36e
\n\t BITOPS_PLUGIN_DIR: [{bitops_plugins_dir}] \ |
that will keep vars a bit cleaner in this function.
Testing
|
scripts/plugins/settings.py
Outdated
@@ -39,6 +39,7 @@ | |||
BITOPS_ENV_run_mode = os.environ.get("BITOPS_MODE") | |||
BITOPS_ENV_logging_level = os.environ.get("BITOPS_LOGGING_LEVEL") | |||
BITOPS_ENV_plugin_dir = os.environ.get("BITOPS_PLUGIN_DIR") | |||
BITOPS_ENV_installed_plugin_dir = os.environ.get("BITOPS_INSTALLED_PLUGIN_DIR") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we agreed to not expose the configuration for this setting via bitops.config.yaml
, can we exclude setting it from the ENV var as a source too?
That's again for the reasons of simplicity and avoiding exposing configuration that's not needed by the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right but plugin scripts directly use this env var ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exporting for the plugins looks fine.
But here we're importing the ENV var from the user's side. This could be removed as we don't want users to override this setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would make BITOPS_INSTALLED_PLUGIN_DIR = /opt/bitops/scripts/installed_plugins/
a constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha 👍 Will fix this after lunch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
Thanks for all the changes!
The requested change to remove the config parameter was addressed
Description
Updating where plugins are installed by updating the path that is used.
fixes #339
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
I tested this locally, proof is in the issue ticket.
Checklist: