-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add multitenant support to script #7
base: master
Are you sure you want to change the base?
Conversation
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.
In general we can replace all long checks like (MULTITENANT_SERVER_VARS_LOCAL_LOCATION is undefined) and check_theme_root_exist.stat.exists and server_vars.stat.exists
with one set_fact and check only 1 variable if it's true.
@@ -0,0 +1,34 @@ | |||
#!/bin/bash | |||
CONFIGURATION_PLAYBOOKS_DIR='/encrypted/ansible/edx/playbooks' | |||
THEMEX_SCRIPT_DIR='/encrypted/ansible/edx/playbooks/themex_script' |
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.
It's not always /encrypted. It can be variable and use default value if not set.
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.
@sidorovdmitry
it's just an example script (example_multitenant_update_theme.sh)
echo "" | ||
else | ||
$THEMEX_SCRIPT_DIR/update_theme.sh -brhim marvel-yellow-theme-eucalyptus https://github.com/raccoongang/themes_for_themex.io.git all_host_edx $CONFIGURATION_PLAYBOOKS_DIR/hosts $CONFIGURATION_PLAYBOOKS_DIR/server-vars.yml | ||
fi |
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.
Why https://github.com/raccoongang/themes_for_themex.io.git ? Move it to variables
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.
@sidorovdmitry
it's just an example script (example_multitenant_update_theme.sh)
set_fact: | ||
server_vars_path: "{{ MULTITENANT_SERVER_VARS }}" | ||
when: MULTITENANT_SERVER_VARS is defined | ||
|
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.
Why do we need 2 equal variables?
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.
@sidorovdmitry
because of playbook multifunctionality. We cannot use server_vars_path variable here
when: MULTITENANT_SERVER_VARS is undefined |
@@ -51,10 +61,11 @@ | |||
- role: branch | |||
theme_branch: "{{ theme_branch }}" | |||
theme_repo: "{{ theme_repo }}" | |||
when: (("{{ script_mode }}" == 'br') or ("{{ script_mode }}" == 'brh') or ("{{ script_mode }}" == 'brhi')) and (script_mode is defined) and (theme_branch is defined) and (theme_repo is defined) | |||
when: (("{{ script_mode }}" == 'br') or ("{{ script_mode }}" == 'brh') or ("{{ script_mode }}" == 'brhi') or ("{{ script_mode }}" == 'brhim')) and (script_mode is defined) and (theme_branch is defined) and (theme_repo is defined) |
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.
theme_branch: "{{ theme_branch|default('master') }}"
theme_repo: "{{ theme_repo|default( some our theme repo ) }}"```
when: script_mode is defined and script_mode in 'brhim' ?
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.
@sidorovdmitry updated with defaults
when: script_mode is defined and script_mode in 'brhim' ? - not applicable (f.e. 'him' in 'brhim' is True but not supported mode)
|
||
- name: Ensure that server-vars contain EDXAPP_COMPREHENSIVE_THEME_DIR for MULTITENANT_SERVER_VARS_LOCAL_LOCATION | ||
local_action: lineinfile dest="{{ server_vars_path }}" | ||
regexp='^EDXAPP_COMPREHENSIVE_THEME_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.
One tab for yaml = 2 spaces
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.
@sidorovdmitry fixed
4b4bbe2
to
92034a2
Compare
@sidorovdmitry all issues are fixed |
7c5b07f
to
119cffa
Compare
@sidorovdmitry
@imatviian
please review