-
Notifications
You must be signed in to change notification settings - Fork 21
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 configure SS and pipeline #162
Conversation
This fixes #161 |
69530b3
to
8a0541e
Compare
You can use the following variables on vars.yml file for testing: archivematica_src_configure_ss_user: "ss_user" To run the playbook with the configure feature, use the extravars:
|
I forgot to mention the more important thing. There's a breaking change in the PR artefactual/archivematica#681 about the dashboard API whitelist: an empty whitelist now means: "allow from all". Do you think that a better default whitelist could be "localhost" or another value? |
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.
Some feedback provided
defaults/main.yml
Outdated
archivematica_src_configure_am_password: "test" # Dashboard admin password | ||
archivematica_src_configure_am_email: "[email protected]" # Dashboard admin password | ||
archivematica_src_configure_am_org_name: "test" # Dashboard admin Org name | ||
archivematica_src_configure_am_org_id: "test" # Dashboard admin Org id |
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.
Can you add also the "First name" and "last name" fields? This way, we have all the configuration options exposed here.
tasks/pipeline-configure.yml
Outdated
|
||
- name: "Create Dashboard user and register pipeline on SS" | ||
django_manage: | ||
command: "install --username={{ archivematica_src_configure_am_user }} --password={{ archivematica_src_configure_am_password }} --email={{ archivematica_src_configure_am_email }} --org-name={{ archivematica_src_configure_am_org_name }} --org-id={{ archivematica_src_configure_am_org_id }} --api-key={{ archivematica_src_configure_am_api_key }} --ss-url={{ archivematica_src_configure_ss_url }} --ss-user={{ archivematica_src_configure_ss_user }} --ss-api-key={{ archivematica_src_configure_ss_api_key }} --whitelist={{archivematica_src_configure_am_whitelist}}" |
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 should be one line for each switch, it would improve readability
tasks/ss-configure.yml
Outdated
- set_fact: archivematica_src_configure_ss_api_key={{ 999999999999999999999 | random | to_uuid | hash('md5') }} | ||
when: archivematica_src_configure_ss_api_key is undefined | ||
|
||
- name: "Create SS superuser" |
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.
What do you think about deleting the default test user when this is set?
tasks/pipeline-configure.yml
Outdated
- set_fact: archivematica_src_configure_am_api_key={{ 999999999999999999998 | random | to_uuid | hash('md5') }} | ||
when: archivematica_src_configure_am_api_key is undefined | ||
|
||
- name: "Create Dashboard user and register pipeline on SS" |
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.
You have archivematica_src_configure_dashboard
and archivematica_src_configure_ss_dashboard: "no"
, but seems like this configures both things always.
defaults/main.yml
Outdated
# Configure AM | ||
# archivematica_src_configure_am_api_key & archivematica_src_configure_ss_api_key can be defined as vars. If not defined they will be autogenerated by the playbook | ||
archivematica_src_configure_ss_user: "test" # SS superuser | ||
archivematica_src_configure_ss_password: "test" # SS superuser password |
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.
I think passwords should be something line "PleaseChangeMe!", so users don't forget to set them
tasks/main.yml
Outdated
# Configure SS | ||
# | ||
|
||
- include: "ss-configure.yml" |
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.
As the files have only two tasks each, what about consolidating them into a single one?
Thanks Santi, I have considered all your suggestions except for entering the variables "First name" and "Last name" The variables "First name" and "Last name" were not contemplated in the artefactual/archivematica#681 PR, so it requires an AM additional change. I have simplified the variables and tags since it could lead to confusion and it made the configuration more complicated. Now there is only the The default SS test user is deleted when adding a custom SS superuser. I have consolidated all task into a single file, called configure.yml. The default password was set to "PleaseChangeMe!" The format is now cleaner introducing some line breaks to facilitate the reading of the code. This PR is ready for CR again. |
3a76a3c
to
5b64f22
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.
Some more comments
tasks/configure.yml
Outdated
become: "yes" | ||
shell: | | ||
cd {{ archviematica_src_ss_app }} | ||
export SS_DB_NAME={{ archivematica_src_ss_environment['SS_DB_NAME'] }} |
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.
I think you can set the env in a line, as we do here:
ansible-archivematica-src/tasks/ss-db.yml
Line 17 in a38b2d1
environment: "{{ archivematica_src_ss_environment }}" |
tasks/configure.yml
Outdated
executable: /bin/bash | ||
when: "archivematica_src_configure_ss|bool" | ||
|
||
#source {{ archivematica_src_ss_virtualenv }}/bin/activate; |
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.
Is this dead code?
I'm thinking about adding a "security" measure, to avoid breaking currently configured systems:
|
@mamedin 🏓 |
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.
Let's try to get this merged? It's a nice feature.
Hi @sevein to merge: -I need to change the PR to take account the last @scollazo's suggestions:
|
@mamedin ops ok I see. Any of that is going to happen for AM17 so I guess we can ignore this PR for now. Thanks! |
Although not idempotent, the changes in this PR work on am 1.7. @mamedin can you address my comments about setting environment vars so we can merge this? We should also create a bug in archivematica in order to get the changes needed for idempotency into 1.7.1, or at least in qa/1.x |
4f6ed81
to
5928a31
Compare
@scollazo It is ready for QA, I have included your suggestions about stting environment. |
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.
Feedback added. What happens if you run this playbook twice? Does the password get reset?
defaults/main.yml
Outdated
archivematica_src_configure_dashboard: "no" | ||
archivematica_src_configure_ss: "no" | ||
|
||
#Components to configure |
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.
Duplicated?
defaults/main.yml
Outdated
archivematica_src_configure_am_user: "test" # Dashboard admin | ||
archivematica_src_configure_am_password: "PleaseChangeMe!" # Dashboard admin password | ||
archivematica_src_configure_am_email: "[email protected]" # Dashboard admin password | ||
archivematica_src_configure_am_org_name: "test" # Dashboard admin Org name |
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.
Use "test org" for org, and "test id" for id?
tasks/configure.yml
Outdated
|
||
# Create api-key if not defined | ||
- set_fact: archivematica_src_configure_am_api_key={{ 999999999999999999998 | random | to_uuid | hash('md5') }} | ||
when: archivematica_src_configure_am_api_key is undefined |
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.
For the ss api key, the condition is when: "archivematica_src_configure_ss_api_key is undefined and archivematica_src_configure_ss|bool"
, but here you are not checking for archivematica_src_configure_dashboard
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.
Yes, it was done intentionally in case just SS is installed.
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.
I'm not getting your point, I think that the other case (deploying the dashboard, with the ss in other server), will also fail.
Thinking out loud , does it make sense to place this steps in pipeline-dbconf.yml for the dashboard, and ss-db.yml for the storage service? This way you can rely on archivematica_src_install_am
and archivematica_src_install_ss
to avoid this issue.
tasks/configure.yml
Outdated
- name: "Delete default SS superuser" | ||
become: "yes" | ||
shell: | | ||
cd {{ archviematica_src_ss_app }} |
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.
I think that using cd
, and chdir
later, is not needed
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.
Also, typo
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.
Yes, this "cd" is not needed. The typo is prior to this change, and this variable is used several times in the role. I'll include the typo fix as a separate commit.
tasks/configure.yml
Outdated
|
||
# Create api-key if not defined | ||
- set_fact: archivematica_src_configure_am_api_key={{ 999999999999999999998 | random | to_uuid | hash('md5') }} | ||
when: archivematica_src_configure_am_api_key is undefined |
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.
I'm not getting your point, I think that the other case (deploying the dashboard, with the ss in other server), will also fail.
Thinking out loud , does it make sense to place this steps in pipeline-dbconf.yml for the dashboard, and ss-db.yml for the storage service? This way you can rely on archivematica_src_install_am
and archivematica_src_install_ss
to avoid this issue.
9753a88
to
37d5e5c
Compare
Pending to the artefactual/archivematica-storage-service#364 (comment) I found some problems that I want to fix:
|
Related: artefactual/archivematica#1131. |
Related: artefactual/archivematica#1134. |
I have updated again this PR. I added 2 basic test looking at the dashboard database to check whether the pipeline is already registered or the dashboard user already exists. With these 2 basic tests we make sure that this role only runs in new deployments, which is where it makes sense. I have rebased again to check the last commits and now there are no problems when using the It is ready for CR again. |
74458ee
to
c46e0e8
Compare
There are new changes that needs to be cr'ed again
Instead of aborting if the user is already created, why not check for it first, and only create it when needed? |
Hi @scollazo , the artefactual/archivematica#1165 PR will make the dashboard installer idempotent, so the last commit could be removed because it is not needed. |
I made a copy of this branch and rebased my copy on top of the recent changes in the master branch of this repo: https://github.com/jhsimpson/ansible-archivematica-src/tree/dev/configure-ss-dashboard I tested this deploying to an Ubunt 16.04 host, using this dev branch of deploy-pub: https://github.com/artefactual/deploy-pub/tree/dev/deploy-am-post17 This worked great for me, on a fresh install. I haven't tested upgrades or other os'es. I did have to play around a bit to figure out what ansible variables I actually needed. You can see the variables I used here:https://github.com/artefactual/deploy-pub/blob/dev/deploy-am-post17/playbooks/archivematica/vars-singlenode-qa.yml @mamedin I think that last commit could stay, it is not bad to check if the users exist, even if the dashboard installer itself is idempotent, it is possible for users to be created by other methods (in db directly, by django shell commands). |
In this last commit I have changed a bit the order of the tasks and checks. Now instead of abort when it detects the pipeline was configured or the pipeline user exists, it does nothing and sends a message. |
aae7f21
to
8b6323a
Compare
This PR will use the features added by the following PRs: -artefactual/archivematica#681 -artefactual/archivematica-storage-service#213 Specifically, this PR allows to: a) Create a SS superuser b) Create a dashboard admin user and register the pipeline on the SS. To configure SS and pipeline on a deployment, the following boolean extra-vars have to be enabled: - archivematica_src_configure_dashboard: To create the dashboard admin user and to register pipeline on SS - archivematica_src_configure_ss: To create SS superuser They are disabled by default.
This commit adds two test to check whether the dashboard user `archivematica_src_configure_am_user` exists and the pipeline is already registered before running the configure tasks. These new tasks will abort the playbook when any of these basic test fails.
8b6323a
to
82d2e7f
Compare
It has been a long way, but imho, this is ready for merge, kudos @mamedin ! We should add those new values to the docs in a separate commit. |
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.
LGTM!
This PR will use the features added by the following PRs:
-artefactual/archivematica#681
-artefactual/archivematica-storage-service#213
Specifically, this PR allows to:
a) Create a SS superuser
b) Create a dashboard admin user and register the pipeline on the SS.
To configure SS and pipeline on a deployment, the following boolean
extra-vars have to be enabled:
to register pipeline on SS
They are disabled by default.
connects to #161