Add repo state and possibility to specify package name #151

javierbertoli · 2018-06-25T18:34:36Z

This PR tried to address #150, adding a separate state to manage repos repo.sls.

When testing, found many issues due to old/deprecated versions/repos, so I tried to refactor the formula to be more up-to-date:

Added support for testing on:
- ubuntu-1804
- debian-stretch
- debian-jessie
Added osfamilymap.yaml
Updated repo info

This probably can fix/close #109, #150, #131, #136 and #95

I'm not using this formula heavily, so if you can check this PR and make suggestions, any comment will be welcome.

It seems that a lot of refactoring is needed to clean old stuff and perhaps this PR helps to start doing it 😄

noelmcloughlin · 2018-06-28T13:26:00Z

test/integration/version-18.03.1/testinfra/test_docker.py

@@ -2,9 +2,9 @@


 def test_package_is_installed(Package):
- docker = Package('docker-engine')
+ docker = Package('docker-ce')


Could the package name be referenced as variable?

See #153 could you use docker:pkg:name here (and elsewhere) please?

Can't use a saltstack pillar var in test python code without non-trivial work

noelmcloughlin · 2018-06-28T13:26:14Z

test/integration/version-18.03.1/testinfra/test_docker.py

 assert docker.is_installed
- assert docker.version.startswith('1.12.2')
+ assert docker.version.startswith('18.03.1')


Could the package version be configurable?

See #153 could you use docker:pkg:version here (and elsewhere) please?

Can't use a saltstack pillar var in test python code without non-trivial work

Adding to what @blbradley says:

I'm particularly reluctanct to use pillar data directly in testing code: I write testing to make sure that the whole formula (states + default pillar data) is correct and does what I expect from it.

To put an example, if I set a "wrong", installable, docker:pkg:version in the formula, like "17.03.1", and test it, It will work OK, tests will pass.

The "states" are right, but the formula will install "17.03.1" and will not be doing what I want.

I don't know if I'm clear in what I mean...

Again, that's me, and I'm OK doing it the way @noelmcloughlin suggest if you prefer that way.

Hi @javierbertoli @blbradley I'm okay with not complicating things so leave this as is!! thx

blbradley · 2018-06-29T11:59:34Z

.travis.yml

@@ -9,4 +9,17 @@ services:
 before_install:
 - bundle install

-script: bundle exec kitchen verify
+
+env:


Are you sure you need this? I think the matrix is controlled by suiteXplatform in Test Kitchen.

blbradley · 2018-06-29T12:00:31Z

I don't really use this formula anymore but will give some suggestions.

blbradley · 2018-06-29T12:05:08Z

What version of Docker does the default suite test these days?

blbradley · 2018-06-29T14:09:11Z

.travis.yml

+ - INSTANCE: default-debian-stretch
+ - INSTANCE: default-debian-jessie
+ - INSTANCE: default-centos-7
+ - INSTANCE: version-1122-ubuntu-1804


Hey I think these names need to start version-18031 now. Right? If you all choose to keep this. This is one reason I let Test Kitchen just control the test matrix.

Yep! See the errors in Travis:

No instances for regex version-1122-ubuntu-1804', try running kitchen list'

blbradley · 2018-06-29T14:12:29Z

Thanks for the quality updates, @javierbertoli. I won't give a 👍 or 👎 on this because I'm not a user of this formula anymore. But I'm happy to answer any questions about the test suite if you hit any trouble.

noelmcloughlin · 2018-07-10T16:17:38Z

Hi @javierbertoli Do you need me to work with you on this?

javierbertoli · 2018-07-10T16:22:58Z

Hey, sorry. I'm having some unrelated, Real Life (TM) issues that left me litte time to advance on this 😋

I have some commits that failed local tests, that I need to review and push. I'll see if I can push them today/tomorrow. Is it ok? Or do you prefer I push them as they are, so we can review them?

noelmcloughlin · 2018-07-10T16:56:57Z

@javierbertoli No worries, in your own time!!! Good luck

javierbertoli · 2018-07-21T23:19:47Z

@noelmcloughlin, @blbradley, updated the PR. Had to remove testing on CentOS, as docker-in-docker fails. Updated to test Debian 8/9 & Ubuntu 18.04.

To keep things backward-ish compatible, kept the use_old_repo parameter & repo, but kept it somewhat 'hardcoded', as the new format is capable of managing all the parameters (repo & package) to install any package from any repo. I think in a future iteration we can remove the old parameters.

I think the formula needs a some cleanup and refactoring (ie, I think that kernels.sls can probably be removed now), but that can be done in future PRs.

Please let me know if you have any suggestions or want anything else done in the PR.

noelmcloughlin · 2018-07-23T15:41:49Z

docker/kernel.sls

@@ -1,10 +1,11 @@
 {% from "docker/map.jinja" import docker with context %}

+{%- if docker.kernel is defined and grains['os_family']|lower == 'debian' %}
 pkgrepo dependencies:
 pkg.installed:
 - name: python-apt


Good fix. There is no python-apt package on non-Debian distros so state fails.

noelmcloughlin · 2018-07-23T15:43:11Z

Should we just dump the use_old_repo support in this PR - because nobody needs this?

javierbertoli · 2018-07-23T15:54:08Z

Don't know. If you ask me, I'd dump it, same with the kernel.sls state file, as those things are not needed anymore. But didn't want to add too much refactoring in this PR (as it already is, is quite big), so I'm OK with adding a few follow ups to clean the formula. Wdyt?

noelmcloughlin · 2018-07-23T15:56:23Z

Yes - followup would be appreciated if someone can make time. This LGTM as is, and future PR can progress things. Thanks @javierbertoli

aboe76 · 2018-07-23T19:11:39Z

@javierbertoli and @noelmcloughlin merged it.

javierbertoli requested review from myoung34, blbradley and noelmcloughlin June 25, 2018 18:34

noelmcloughlin reviewed Jun 28, 2018

View reviewed changes

noelmcloughlin mentioned this pull request Jun 28, 2018

Use pkg installed (not latest), allow_updates, version #153

Merged

blbradley reviewed Jun 29, 2018

View reviewed changes

noelmcloughlin mentioned this pull request Jul 11, 2018

add pip packagename for freebsd os #158

Merged

javierbertoli force-pushed the master branch 2 times, most recently from e81c6a7 to 2dc17b5 Compare July 21, 2018 19:43

Add new state to manage repo, update defaults

3652849

javierbertoli force-pushed the master branch from 2dc17b5 to 3652849 Compare July 21, 2018 19:48

kernel.sls is only valid for Debian family

a905257

javierbertoli force-pushed the master branch from cf862d6 to a905257 Compare July 21, 2018 20:15

Centos docker-in-docker is not supported for testing

65023b6

javierbertoli force-pushed the master branch from d181141 to 65023b6 Compare July 21, 2018 22:00

noelmcloughlin reviewed Jul 23, 2018

View reviewed changes

noelmcloughlin approved these changes Jul 23, 2018

View reviewed changes

aboe76 merged commit 0827056 into saltstack-formulas:master Jul 23, 2018

javierbertoli mentioned this pull request Aug 10, 2018

Replacing APT repo #150

Closed

noelmcloughlin mentioned this pull request Feb 18, 2019

Revert two issues introduced by #151 #195

Merged

aboe76 pushed a commit that referenced this pull request Feb 20, 2019

Revert PR #151 minor issues (#195)

c1c1643

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add repo state and possibility to specify package name #151

Add repo state and possibility to specify package name #151

javierbertoli commented Jun 25, 2018 •

edited

Loading

noelmcloughlin Jun 28, 2018 •

edited

Loading

noelmcloughlin Jun 28, 2018 •

edited

Loading

blbradley Jun 29, 2018

noelmcloughlin Jun 28, 2018

noelmcloughlin Jun 28, 2018

blbradley Jun 29, 2018

javierbertoli Jun 29, 2018

noelmcloughlin Jun 29, 2018

blbradley Jun 29, 2018

blbradley commented Jun 29, 2018

blbradley commented Jun 29, 2018

blbradley Jun 29, 2018

blbradley Jun 29, 2018

blbradley commented Jun 29, 2018

noelmcloughlin commented Jul 10, 2018

javierbertoli commented Jul 10, 2018

noelmcloughlin commented Jul 10, 2018

javierbertoli commented Jul 21, 2018 •

edited

Loading

noelmcloughlin Jul 23, 2018

noelmcloughlin commented Jul 23, 2018

javierbertoli commented Jul 23, 2018

noelmcloughlin commented Jul 23, 2018

aboe76 commented Jul 23, 2018

Add repo state and possibility to specify package name #151

Add repo state and possibility to specify package name #151

Conversation

javierbertoli commented Jun 25, 2018 • edited Loading

noelmcloughlin Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

noelmcloughlin Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blbradley commented Jun 29, 2018

blbradley commented Jun 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blbradley commented Jun 29, 2018

noelmcloughlin commented Jul 10, 2018

javierbertoli commented Jul 10, 2018

noelmcloughlin commented Jul 10, 2018

javierbertoli commented Jul 21, 2018 • edited Loading

Choose a reason for hiding this comment

noelmcloughlin commented Jul 23, 2018

javierbertoli commented Jul 23, 2018

noelmcloughlin commented Jul 23, 2018

aboe76 commented Jul 23, 2018

javierbertoli commented Jun 25, 2018 •

edited

Loading

noelmcloughlin Jun 28, 2018 •

edited

Loading

noelmcloughlin Jun 28, 2018 •

edited

Loading

javierbertoli commented Jul 21, 2018 •

edited

Loading