-
Notifications
You must be signed in to change notification settings - Fork 71
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
Make server_configuration optionnal. #1736
Make server_configuration optionnal. #1736
Conversation
Make server_configuration optional for proxy and client.
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 don't see a problem with this
The build host, build docker images. @Bischoff could you bring some light before approving this PR please? |
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.
Waiting @Bischoff feedback about avahi_reflector support in a Build Host
modules/build_host/main.tf
Outdated
@@ -19,9 +19,6 @@ module "build_host" { | |||
disable_firewall = var.disable_firewall | |||
grains = { | |||
mirror = var.base_configuration["mirror"] | |||
server = var.server_configuration["hostname"] | |||
auto_connect_to_master = var.auto_connect_to_master |
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.
From the point of view of auto_connect_to_master
feature, I think it make sense to have the possibility to auto connect a build host, at the end is just another salt minion that we onboard with a special Add-On system type.
Instead of removing it because is unused at that moment, I would consider to change its parent from host
to minion
. So that would make possible for a build_host to auto-connect to master.
If you agree on that, we can create a card to track it and don't forget.
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 just that it has never been used for a long while ( maybe ever ). I was thinking adding the auto connect to build host, but if it's not needed now, I don't see a need for later.
I will ping Pablo to have a look at this PR.
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 used to use avahi quite a lot. These days a lot less, because of our private DNS entries, but it's a privilege. I know at least 2 developers who rely on avahi.
We don't need auto connect on the build host (*), but we do need the avahi reflector.
(*) at least not me, but maybe some developer or supporter does. Oscar mentioned that in demoes our supporters use that auto connect thing intensively.
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 would agree with Óscar on moving the build_host
from host
to minion
and then keeping the auto-connect with master feature.
As developer, I use avahi on my deployments all the times, OTOH I never use this reflector config, which seems not be actually working at the moment for build_host
(as only does stuff on minion SLS files)
Do we have a use case ? Sumaform is already complex, by digging I found those unused variables. It has been like that for a while. |
If you deploy a build host on your workstation where you use avahi, you will need the reflector. I would call this a use case. We don't see it in our official test suites because they all rely on a DNS server. I think Oscar is correct here. Either don't apply, or refactor to be able to use it on a build host. |
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.
same concerns as Oscar
What does this PR change?
- build hosts:- auto connection to master is not possible for build host during salt states => removeserver_configuration
andauto_connect_to_master
variables-avahi_reflector
is only setup in minion salt states.auto_connect_to_master
variableserver_configuration
optional for proxy, client and build_host, this value is only used if respectivelyauto_configure
orauto_register
is set to true.