-
Notifications
You must be signed in to change notification settings - Fork 45
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
Update docs and CI to support rabbitmq-external chart #655
Conversation
offline/federation_preparation.md
Outdated
|
||
There are two ways to deploy the RabbitMQ cluster: | ||
|
||
Method 1 |
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 heading declarations are strange here: See inline fixes.
@julialongtin @supersven As of now, we have multiple pull requests created while making a release for Mandarin, this pr has all the required changes at one place. |
Hey @amitsagtani97 👋 What's wrong about having small, dedicated PRs? I would prefer them. If you mixed several topics together, could you please update the title and description of this PR? |
ansible/helm_external.yml
Outdated
external_dir_name: rabbitmq-external | ||
server_type: rmq-cluster | ||
network_interface: "{{ rabbitmq_network_interface }}" | ||
# - hosts: "rmq-cluster" |
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 commenting out the best way to not execute this task? Wouldn't it be better to rely on some conditional (e.g. when: rabbitmq_cluster.is_internal == false
(these value names likely don't exist yet, I made them up))?
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.
Created a tag for the rabbimq role and added --skip-tags and --tags to install as required.
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.
Unfortunately, this needs some overhauling.
bin/offline-helm.sh
Outdated
set -x | ||
|
||
SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" | ||
ANSIBLE_DIR="$( cd "$SCRIPT_DIR/../ansible" && pwd )" | ||
ansible-playbook -i "$ANSIBLE_DIR"/inventory/offline "$ANSIBLE_DIR"/helm_external.yml --skip-tags=rabbitmq-external -vv |
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 did we need to move this here? bin/offline-helm.sh
and bin/offline-cluster.sh
are both called by bin/offline-deploy.sh
in order?. 🤔
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.
Previously when I did not had the individual tag for the rabbitmq-external role in the helm_external.yml. I had to do it this way, because offline-cluster.sh script is also used by us to create a test env in the Hetzner machine and we wanted to avoid the rabbitmq-external role there.
But now after the recent changes, I have safely removed it from offline-helm and moved back to offline-cluster.sh
@amitsagtani97 The code is looking good now. But, IMHO the commit history should be cleaned up a bit: It would be great if you could squash and reword the commit history such that the try&error commits go away... 😸 |
c1e2651
to
3ba71f7
Compare
https://wearezeta.atlassian.net/browse/WPB-5886
As of now, there are multiple prs created for the Mandarin release in the past, which were based on top of rabbitmq integration pr. Each of those prs has some minor changes or other fixes as well.
Pulling those changes into one pr, as all the changes together makes the CI green and have the required docs changes to support rabbitmq-external chart.