Add paygo product version condition to skip useless repositories #1428
Conversation
maximenoel8
requested review from
vandabarata,
Bischoff,
srbarrios,
nodeg,
ktsamis and
NamelessOne91
| @@ -3,7 +3,6 @@ | |||
|
|
|||
| include: | |||
| - scc | |||
| - repos | |||
There was a problem hiding this comment.
Shouldn't we have here this?
- repos.testsuite
There was a problem hiding this comment.
The repos.testsuite will be call during default > minimal. I try to remove useless call to repo.
There was a problem hiding this comment.
If one of the salt states of this sls requires another sls, I think it would make sense to include it here.
I would like the opinion of a Salt expert for this doubt.
There was a problem hiding this comment.
IIRC what is on minimal should also be present in the general highstate. Somaone can call the script highstate.sh which runs the highstate only, and should have the same behaviour.
There was a problem hiding this comment.
If we do a quick search inside the salt folder, we will notice that for:
- require:
- sls:
We always include the sls that is required afterward, I think that we should keep this in common unless there is a good reason to do it differently here.
There was a problem hiding this comment.
Please before resolving this, let's finish the discussion.
There was a problem hiding this comment.
From a Salt PoV, IIRC the require works as long as repos.testsuite (in this case) is part of the sls files that are looked at. But from a sumaform developer PoV, I would always include it where I need it. Relying on global state is a general code smell.
salt/minion/init.sls
Outdated
| @@ -1,6 +1,6 @@ | |||
| include: | |||
| - scc.minion | |||
| {% if 'build_image' not in grains.get('product_version') | default('', true) %} | |||
| {% if 'build_image' and 'paygo' not in grains.get('product_version') | default('', true) %} | |||
There was a problem hiding this comment.
Can we check two values at same time?
I think it should be:
| {% if 'build_image' and 'paygo' not in grains.get('product_version') | default('', true) %} | |
| {% if 'build_image' not in grains.get('product_version') and 'paygo' not in grains.get('product_version') | default('', true) %} |
There was a problem hiding this comment.
I tested this expression and it works. Do you want to have it in our format ?
There was a problem hiding this comment.
It works just because the first part it returns always true.
{% if 'build_image' %}
true
{% endif %}
You can test it here: https://j2live.ttl255.com/
| @@ -3,7 +3,6 @@ | |||
|
|
|||
| include: | |||
| - scc | |||
| - repos | |||
There was a problem hiding this comment.
IIRC what is on minimal should also be present in the general highstate. Somaone can call the script highstate.sh which runs the highstate only, and should have the same behaviour.
| @@ -14,7 +13,7 @@ default_cucumber_requisites: | |||
| - milkyway-dummy | |||
| - virgo-dummy | |||
| - require: | |||
| - sls: repos | |||
| - sls: repos.testsuite | |||
There was a problem hiding this comment.
In pkg.installed, the packages are coming from repos.testsuite. I'm not an salt expert but I find it easier to read if we point directly to the needed resource and not to a folder with 20+ files.
Also, for my paygo minion, I want those repositories but not the other repositories.
Because of the sumaform architecture, it's hard to split those resources. Calling the specific salt file solves the issue without adding extra condition around the requirement.
There was a problem hiding this comment.
Keep in mind that require is just for ordering the states. pkg.installed takes a fromrepo argument if you want to specify which repo to actually use when installing things. Of course, if there is only one repo that has a package, that's automatically used by zypper.
|
Thanks for the reviews, I still need a new deployment to test the default value. I introduced this condition 2 years ago so I don't remember completely. From my memory, I struggled a bit with this condition. But it requires some extra testing. |
maximenoel8
force-pushed
the
add_support_paygo_image_salt_states
branch
from
78a76c3 to
660bfea
Compare
|
The default value is required or you have this error : |
| @@ -3,7 +3,6 @@ | |||
|
|
|||
| include: | |||
| - scc | |||
| - repos | |||
There was a problem hiding this comment.
From a Salt PoV, IIRC the require works as long as repos.testsuite (in this case) is part of the sls files that are looked at. But from a sumaform developer PoV, I would always include it where I need it. Relying on global state is a general code smell.
| @@ -14,7 +13,7 @@ default_cucumber_requisites: | |||
| - milkyway-dummy | |||
| - virgo-dummy | |||
| - require: | |||
| - sls: repos | |||
| - sls: repos.testsuite | |||
There was a problem hiding this comment.
Keep in mind that require is just for ordering the states. pkg.installed takes a fromrepo argument if you want to specify which repo to actually use when installing things. Of course, if there is only one repo that has a package, that's automatically used by zypper.
What does this PR change?
Repositories
Paygo instances on cloud ( testing with AWS ) already have their repositories by default.
Using internal product repositories doesn't make sense for paygo testing.
Currently to have access to products repositories we have two solutions :
In my changes, I introduce a new product version extension call paygo. If you have paygo in your product version name, it will disable salt repo and scc
Nevertheless, we still need the testsuite repositories for the minion, so I only keep respo.testsuite call.
I also removed a redundant repo call in testsuite instead of adding condition around it.
By following the salt paths, I find out that by default we call repo three time.
Server installation
Server packages are already installed and don't need to be install again.