Skip to content
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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

maximenoel8
Copy link
Contributor

@maximenoel8 maximenoel8 commented Oct 9, 2024

What does this PR change?

  • Remove variables not impacting the deployment.
    - build hosts:
    - auto connection to master is not possible for build host during salt states => remove server_configuration and auto_connect_to_master variables
    - avahi_reflector is only setup in minion salt states.
    • ssh minion:
      • auto connection to master is not possible for ssh minion during salt states => remove auto_connect_to_master variable
  • Make server_configuration optional for proxy, client and build_host, this value is only used if respectively auto_configure or auto_register is set to true.

@maximenoel8 maximenoel8 changed the title Remove variables not impacting the deployment. Remove variables unused during sumaform deployment. Oct 10, 2024
Copy link
Contributor

@ktsamis ktsamis left a 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

@srbarrios
Copy link
Member

srbarrios commented Oct 10, 2024

The build host, build docker images.
Looking at salt states, you are right, we don't seem to have avahi_reflector used anywhere else aside minion, but... maybe we should do the opposite of the proposed change and support avahi_reflector in build_host, to do so, I think it's enough to change its parent from host to minion.

@Bischoff could you bring some light before approving this PR please?

Copy link
Member

@srbarrios srbarrios left a 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

@@ -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
Copy link
Member

@srbarrios srbarrios Oct 10, 2024

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@Bischoff Bischoff Oct 10, 2024

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.

Copy link
Member

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)

@maximenoel8
Copy link
Contributor Author

The build host, build docker images. Looking at salt states, you are right, we don't seem to have avahi_reflector used anywhere else aside minion, but... maybe we should do the opposite of the proposed change and support avahi_reflector in build_host, to do so, I think it's enough to change its parent from host to minion.

@Bischoff could you bring some light before approving this PR please?

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.

@Bischoff
Copy link
Contributor

The build host, build docker images. Looking at salt states, you are right, we don't seem to have avahi_reflector used anywhere else aside minion, but... maybe we should do the opposite of the proposed change and support avahi_reflector in build_host, to do so, I think it's enough to change its parent from host to minion.
@Bischoff could you bring some light before approving this PR please?

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.

Copy link
Contributor

@Bischoff Bischoff left a 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

@maximenoel8 maximenoel8 changed the title Remove variables unused during sumaform deployment. Make server_configuration optionnal. Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants