Skip to content

Conversation

@tomassedovic
Copy link
Contributor

Depending on the deployment (with or without a loadbalancer, with or
without external DNS) the openshift_master_cluster_hostname may be
different. And given the OpenStack's hostname suffix behaviour, it is
almost never set to the right value on its own.

This updates the Heat templates and the dynamic inventory to set the
variable to a value that should always work and it can always be
overridden in the group_vars .

@tomassedovic tomassedovic requested a review from tzumainn April 17, 2018 09:32
@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 17, 2018
@tomassedovic
Copy link
Contributor Author

@tzumainn this is pretty important. Without it, we have to set openshift_master_cluster_hostname explicitly (which is not possible e.g. in the case where we don't have a DNS) for a deployment to succeed.

@tzumainn
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2018
value: { get_attr: [loadbalancer, resource.0, private_ip] }
{% else %}
value: { get_attr: [masters, resource.0, private_ip] }
{% endif %}
Copy link
Contributor

Choose a reason for hiding this comment

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

well done!

@tzumainn
Copy link
Contributor

I hit an unrelated error testing this, so I think this one is fine. Thanks!

@tzumainn
Copy link
Contributor

/lgtm

@tomassedovic
Copy link
Contributor Author

/test all

@tomassedovic
Copy link
Contributor Author

Oh, so that still doesn't run the travis and atomic tests? Sigh.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 18, 2018
@tomassedovic
Copy link
Contributor Author

@tzumainn could you add your LGTM back please? I've rebased it to get the CI jobs unstuck. The code didn't change.

@tzumainn
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 18, 2018
@sdodson
Copy link
Member

sdodson commented Apr 19, 2018

/retest

@sdodson
Copy link
Member

sdodson commented Apr 19, 2018

bot, retest this please

@sdodson
Copy link
Member

sdodson commented Apr 19, 2018

/test gcp-upgrade

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 20, 2018
@tomassedovic
Copy link
Contributor Author

This is just a (clean) rebase.

@tzumainn
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 20, 2018
@tomassedovic
Copy link
Contributor Author

bot, retest this please

@tomassedovic
Copy link
Contributor Author

Hm okay so that doesn't seem to do anything when I say it.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 24, 2018
@tomassedovic
Copy link
Contributor Author

Rebased.

Depending on the deployment (with or without a loadbalancer, with or
without external DNS) the `openshift_master_cluster_hostname` may be
different. And given the OpenStack's hostname suffix behaviour, it is
almost never set to the right value on its own.

This updates the Heat templates and the dynamic inventory to set the
variable to a value that should always work and it can always be
overridden in the group_vars .
@tomassedovic
Copy link
Contributor Author

Fixed the tox failure.

@tomassedovic
Copy link
Contributor Author

@tzumainn can I have the lgtm back? The tests are all actually passing now :-).

@tzumainn
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 24, 2018
@openshift-merge-robot openshift-merge-robot merged commit 0fbdb63 into openshift:master Apr 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants