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

Don't trigger server deployment when changing the additional repos #1646

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maximenoel8
Copy link
Contributor

@maximenoel8 maximenoel8 commented Aug 9, 2024

What does this PR change?

Context

Currently, when the additional_repos parameter is added, removed, or modified in a server declaration, sumaform triggers the Salt stages. However, these stages run without actually adding the new repositories. Moreover, this process unnecessarily scans all repository files, which can take several hours, without making any meaningful changes.

After reviewing various pipeline scenarios, I found no use case where triggering the Salt event is necessary when modifying additional_repos. This behavior is particularly problematic in the redeployment feature I'm working on for BV (see PR #1292), as it significantly slows down the redeployment process without any tangible benefit.

Solution

To address this, I propose that we stop triggering the server deployment when changes are made to the additional repositories. This adjustment will streamline the process, eliminating unnecessary delays in deployment.

@maximenoel8 maximenoel8 self-assigned this Aug 9, 2024
@maximenoel8 maximenoel8 marked this pull request as draft August 9, 2024 00:57
@maximenoel8 maximenoel8 requested a review from a team August 13, 2024 22:35
@maximenoel8 maximenoel8 marked this pull request as ready for review August 13, 2024 22:36
@maximenoel8 maximenoel8 changed the title Don't trigger server deployment when changing the additional reposito… Don't trigger server deployment when changing the additional repos Aug 13, 2024
@nodeg
Copy link
Member

nodeg commented Aug 14, 2024

Currently, when the additional_repos parameter is added, removed, or modified in a server declaration, sumaform triggers the Salt stages. However, these stages run without actually adding the new repositories.

Interesting. I did not know that. I would have thought that it works properly.
I have 2 additional questions:

  • Is this the right place to remove the parameter? With your changes, will it not be removed for everything? What about removing it only from the server module/definition?
  • I am not sure about this one, but do we not use this parameter when we inject the MU repos in the BV? Or am I mixing this up?

@maximenoel8
Copy link
Contributor Author

maximenoel8 commented Aug 15, 2024

Good remarks @nodeg .
Some points :

  • Those triggers are only for existing deployments.
  • The additional repository parameter only impact the server ( manage by terracumber.terraformer.inject_repos ).
  • If you use taint, it's going to delete completely the server .
  • Indeed, this change is going to impact all deployment.
  • For me this trigger could be a bait. Those repositories are going to be added to the server, salt is going to update the packages BUT if I'm correct, we don't have update / upgrade method in salt that's going to correctly update the server ( restart spacewalk, etc ).
spacewalk-service:
  service.running:
    - name: spacewalk.target
    - watch:
      - file: spacewalk_search_config
    - require:
      - file: spacewalk_search_config

I may have find an other way to go around this trigger without removing it. Let's move this PR into draft. I will probably close it if my other method works.

@maximenoel8 maximenoel8 marked this pull request as draft August 15, 2024 03:39
@nodeg
Copy link
Member

nodeg commented Aug 15, 2024

Thank you for the detailed explanation and insight, Maxime.

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.

2 participants