Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fixes #20857 - override post_sync_url #211

Merged
merged 1 commit into from
Sep 7, 2017

Conversation

sean797
Copy link
Member

@sean797 sean797 commented Sep 5, 2017

No description provided.

@ekohl
Copy link
Member

ekohl commented Sep 5, 2017

What do you think about using $::foreman::foreman_url? An alternative is re-using $::foreman::servername. It's messy we have both and I'd prefer $::foreman::foreman_url. The benefit is that when you'd include foreman::cli on the same host you automatically configure that correct (though I don't think katello-installer exposes that option).

@sean797
Copy link
Member Author

sean797 commented Sep 5, 2017

@ekohl Yea, nice idea. Updated to use $::foreman::foreman_url Thanks!

include ::certs
include ::certs::apache
include ::certs::foreman
include ::certs::pulp_client

$application_host = $::foreman::foreman_url
Copy link
Member

Choose a reason for hiding this comment

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

It might simplify the config template if you define $post_sync_url here.

@sean797
Copy link
Member Author

sean797 commented Sep 5, 2017

Hows that @ekohl ?

include ::certs
include ::certs::apache
include ::certs::foreman
include ::certs::pulp_client

$post_sync_url = "${$::foreman::foreman_url}${deployment_url}/api/v2/repositories/sync_complete?token=${post_sync_token}"
$application_host = $::foreman::foreman_url
Copy link
Member

Choose a reason for hiding this comment

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

Is this used anywhere?

@ekohl
Copy link
Member

ekohl commented Sep 5, 2017

This looks a lot like a change I already wanted to make but hadn't gotten around to :)

@sean797
Copy link
Member Author

sean797 commented Sep 5, 2017

ah, damn! That was left over from before. Thanks @ekohl

include ::certs
include ::certs::apache
include ::certs::foreman
include ::certs::pulp_client

$post_sync_url = "${$::foreman::foreman_url}${deployment_url}/api/v2/repositories/sync_complete?token=${post_sync_token}"
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised this works. Usually we have ${::foreman::foreman_url}. The tests appear to pass so this is valid syntax?

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting, it must be. I was actually a typo so I've update to match the rest of our code base.

@ekohl ekohl merged commit 451530b into theforeman:master Sep 7, 2017
@ekohl
Copy link
Member

ekohl commented Sep 7, 2017

Thanks! This removes an item from my (mental) list of things to fix.

@sean797 sean797 deleted the post_sync_url branch September 7, 2017 12:10
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants