Handle various architecture formats in aptpkg module #60986

Ch3LL · 2021-10-01T14:56:21Z

What does this PR do?

Handles all repo formats in the aptpkg module

What issues does this PR fix or reference?

Fixes: #60971

Previous Behavior

The architecture repo format [arch=amd64 ] was not parsed correctly.

New Behavior

[arch=amd64 ] is parsed correctly

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

[NA ] Docs
Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
Tests written/updated

myii · 2021-10-02T05:46:54Z

@Ch3LL With respect to testing this with the apt-formula (#60971), this appears to be an improvement since all the states are passing now. However, still got verification failures for all instances:

https://gitlab.com/myii/apt-formula/-/pipelines/381257149

The specific failures are still related to the pkgrepo.managed states:

-----> Verifying <repositories-debian-10-tiamat-py3>...
       Loaded repositories 
Profile: apt formula (repositories)
Version: (not specified)
Target:  ssh://kitchen@docker:49153
  ×  Apt repositories: should be configured (2 failed)

     ...

     ×  File /etc/apt/sources.list.d/multimedia-stable-binary.list content is expected to match /deb \[arch=amd64\] http:\/\/www.deb-multimedia.org stable main/
     expected "" to match /deb \[arch=amd64\] http:\/\/www.deb-multimedia.org stable main/
     Diff:
     @@ -1 +1 @@
     -/deb \[arch=amd64\] http:\/\/www.deb-multimedia.org stable main/
     +""

     ...

     ×  File /etc/apt/sources.list.d/heroku-binary.list content is expected to match /deb \[arch=amd64\] https:\/\/cli-assets.heroku.com\/apt .\//
     expected "" to match /deb \[arch=amd64\] https:\/\/cli-assets.heroku.com\/apt .\//
     Diff:
     @@ -1 +1 @@
     -/deb \[arch=amd64\] https:\/\/cli-assets.heroku.com\/apt .\//
     +""
...
Test Summary: 15 successful, 2 failures, 0 skipped

Essentially, both of the files are empty!

These are the actual rendered states which are failing (to provide the content to the respective file):

...

       deb multimedia-stable:
         pkgrepo.managed:
           - name: deb [arch=amd64  ] http://www.deb-multimedia.org stable main
           - file: /etc/apt/sources.list.d/multimedia-stable-binary.list
           - keyid: 5C808C2B65558117
           - keyserver: keyserver.ubuntu.com
           - clean_file: true
           - refresh: False
           - refresh_db: False
           - onchanges_in:
             - module: apt.refresh_db

...

       deb heroku:
         pkgrepo.managed:
           - name: deb [arch=amd64  ] https://cli-assets.heroku.com/apt ./ 
           - file: /etc/apt/sources.list.d/heroku-binary.list
           - key_url: https://cli-assets.heroku.com/apt/release.key
           - clean_file: true
           - refresh: False
           - refresh_db: False
           - onchanges_in:
             - module: apt.refresh_db

Ch3LL · 2021-10-05T12:14:11Z

@myii thanks for testing, I appreciate it. I just pushed up a fix, can you test again?

myii

@Ch3LL Tried this out on top of the 3004rc1 Tiamat packages. Almost there now, just a minor issue.

https://gitlab.com/myii/apt-formula/-/pipelines/381257149

     ×  File /etc/apt/sources.list.d/multimedia-stable-binary.list content is expected to match /deb \[arch=amd64\] http:\/\/www.deb-multimedia.org stable main/
     expected "deb [arch=amd64]  http://www.deb-multimedia.org stable main\n" to match /deb \[arch=amd64\] http:\/\/www.deb-multimedia.org stable main/
     Diff:
     @@ -1 +1 @@
     -/deb \[arch=amd64\] http:\/\/www.deb-multimedia.org stable main/
     +deb [arch=amd64]  http://www.deb-multimedia.org stable main

There are two spaces after [arch=amd64] instead of the one space expected.

Used the fix I've suggested inline (below).

https://gitlab.com/myii/apt-formula/-/pipelines/383147404

All except Debian 11 are now passing.
- Failing with An error was encountered while installing package(s): apt-get: /opt/saltstack/salt/run/libstdc++.so.6: version 'GLIBCXX_3.4.26' not found (required by /usr/lib/x86_64-linux-gnu/libapt-pkg.so.6.0), which is unrelated to this PR.

salt/modules/aptpkg.py

myii · 2021-10-06T14:37:44Z

@Ch3LL For a wider test, I've used the Tiamat pre-salted images for our weekly testing (that's usually done against the master branch pre-salted images). I've hit the following issue across a number of repos.

https://gitlab.com/myii/grafana-formula/-/jobs/1653421022

                 ID: grafana-package-install-pkg-installed
           Function: pkg.installed
               Name: grafana
             Result: False
            Comment: An error was encountered while installing package(s): W: GPG error: https://repo.saltproject.io/salt-dev/py3/debian/10/amd64 stable InRelease: The following signatures couldn't be verified because the public key is not available: NO_PUBKEY 0E08A149DE57BFBE
              E: The repository 'https://repo.saltproject.io/salt-dev/py3/debian/10/amd64 stable InRelease' is not signed.

It looks like SALTSTACK-GPG-KEY.pub is missing from https://repo.saltproject.io/salt-dev/py3/debian/10/amd64/ (and the other Debian-based platforms as well).

Ch3LL · 2021-10-06T14:42:23Z

@bryceml can you look into the missing key issues ^

Ch3LL · 2021-10-06T14:59:35Z

@myii can you re-review/test? Looks like the tests are passing now too :)

myii · 2021-10-06T15:01:58Z

@Ch3LL For a wider test, I've used the Tiamat pre-salted images for our weekly testing (that's usually done against the master branch pre-salted images). I've hit the following issue across a number of repos.

https://gitlab.com/myii/grafana-formula/-/jobs/1653421022
                 ID: grafana-package-install-pkg-installed
           Function: pkg.installed
               Name: grafana
             Result: False
            Comment: An error was encountered while installing package(s): W: GPG error: https://repo.saltproject.io/salt-dev/py3/debian/10/amd64 stable InRelease: The following signatures couldn't be verified because the public key is not available: NO_PUBKEY 0E08A149DE57BFBE
              E: The repository 'https://repo.saltproject.io/salt-dev/py3/debian/10/amd64 stable InRelease' is not signed.
It looks like SALTSTACK-GPG-KEY.pub is missing from https://repo.saltproject.io/salt-dev/py3/debian/10/amd64/ (and the other Debian-based platforms as well).

@bryceml has explained the situation to me in Slack. This is an issue with pkgrepo.managed state, as outlined in #59785.

myii · 2021-10-06T15:02:29Z

@myii can you re-review/test? Looks like the tests are passing now too :)

@Ch3LL No problem, I'll do it again with apt-formula, since I can't test much wider than that at the moment ^.

waynew

Looks good - couple of questions but I don't think they're blockers

salt/modules/aptpkg.py

myii · 2021-10-06T15:41:41Z

@myii can you re-review/test? Looks like the tests are passing now too :)

@Ch3LL All passing, thanks:

https://gitlab.com/myii/apt-formula/-/pipelines/383671960

bryceml · 2021-10-06T17:30:12Z

@bryceml can you look into the missing key issues ^

going forward, salt-archive-keyring.gpg should be used (binary format), since that is what debian says to do.

waynew

🚀

Ch3LL · 2021-10-08T10:44:13Z

@myii I made a couple more changes. Would you be willing to test this out one more time? I'll merge for now, but ping me on this PR, if you find any issues.

myii · 2021-10-08T15:01:46Z

@myii I made a couple more changes. Would you be willing to test this out one more time? I'll merge for now, but ping me on this PR, if you find any issues.

@Ch3LL I've re-run the tests based on https://raw.githubusercontent.com/saltstack/salt/freeze/salt/modules/aptpkg.py.

https://gitlab.com/myii/apt-formula/-/pipelines/385124356
- All looking good!

Ch3LL · 2021-10-08T15:02:30Z

thank you so much!

Handle various architecture formats in aptpkg module

cbfda84

Ch3LL requested a review from a team as a code owner October 1, 2021 14:56

Ch3LL requested review from joechainz and removed request for a team October 1, 2021 14:56

Ch3LL mentioned this pull request Oct 1, 2021

[BUG] aptpkg.py not handling all repo formats #60971

Closed

myii added a commit to myii/apt-formula that referenced this pull request Oct 2, 2021

ci(tiamat): test proposed saltstack/salt#60986

93e2a8a

Ch3LL added the Silicon v3004.0 Release code name label Oct 4, 2021

Write file even if does not exist

d4f6c61

only run test on debian based platforms

d581a8f

myii added a commit to myii/apt-formula that referenced this pull request Oct 6, 2021

ci(kitchen): test fix saltstack/salt#60986

f04dc08

myii reviewed Oct 6, 2021

View reviewed changes

salt/modules/aptpkg.py Outdated Show resolved Hide resolved

myii added a commit to myii/apt-formula that referenced this pull request Oct 6, 2021

ci(kitchen): test fix saltstack/salt#60986

9141e67

myii added a commit to myii/apt-formula that referenced this pull request Oct 6, 2021

ci(kitchen): test fix saltstack/salt#60986

4bc0b05

Megan Wilhite added 2 commits October 6, 2021 07:42

remove extra space for arch

641b8dc

convert pathlib to string for pkgrepo test

1ebe31a

myii mentioned this pull request Oct 6, 2021

[FEATURE REQUEST] pkgrepo should follow current Debian standards #59785

Closed

myii added a commit to myii/apt-formula that referenced this pull request Oct 6, 2021

ci(tiamat): test proposed saltstack/salt#60986

c512b25

garethgreenaway previously approved these changes Oct 6, 2021

View reviewed changes

waynew previously approved these changes Oct 6, 2021

View reviewed changes

salt/modules/aptpkg.py Outdated Show resolved Hide resolved

salt/modules/aptpkg.py Outdated Show resolved Hide resolved

s0undt3ch previously approved these changes Oct 6, 2021

View reviewed changes

Use temporary files first then copy to sources files

30f0827

Ch3LL dismissed stale reviews from s0undt3ch, waynew, and garethgreenaway via 30f0827 October 7, 2021 16:24

waynew approved these changes Oct 7, 2021

View reviewed changes

Ch3LL merged commit 832764b into saltstack:freeze Oct 8, 2021

myii added a commit to myii/apt-formula that referenced this pull request Oct 8, 2021

ci(tiamat): test merged saltstack/salt#60986

a7abb0e

myii mentioned this pull request Jan 5, 2022

[BUG] things that require python3-apt in salt don't work in tiamat, nor in otherwise-isolated python envs running salt #58555

Closed

Handle various architecture formats in aptpkg module #60986

Handle various architecture formats in aptpkg module #60986

Ch3LL commented Oct 1, 2021

myii commented Oct 2, 2021 •

edited

Ch3LL commented Oct 5, 2021

myii left a comment •

edited

myii commented Oct 6, 2021

Ch3LL commented Oct 6, 2021

Ch3LL commented Oct 6, 2021

myii commented Oct 6, 2021

myii commented Oct 6, 2021

waynew left a comment

myii commented Oct 6, 2021

bryceml commented Oct 6, 2021

waynew left a comment

Ch3LL commented Oct 8, 2021

myii commented Oct 8, 2021

Ch3LL commented Oct 8, 2021

Handle various architecture formats in aptpkg module #60986

Handle various architecture formats in aptpkg module #60986

Conversation

Ch3LL commented Oct 1, 2021

What does this PR do?

What issues does this PR fix or reference?

Previous Behavior

New Behavior

Merge requirements satisfied?

myii commented Oct 2, 2021 • edited

Ch3LL commented Oct 5, 2021

myii left a comment • edited

Choose a reason for hiding this comment

myii commented Oct 6, 2021

Ch3LL commented Oct 6, 2021

Ch3LL commented Oct 6, 2021

myii commented Oct 6, 2021

myii commented Oct 6, 2021

waynew left a comment

Choose a reason for hiding this comment

myii commented Oct 6, 2021

bryceml commented Oct 6, 2021

waynew left a comment

Choose a reason for hiding this comment

Ch3LL commented Oct 8, 2021

myii commented Oct 8, 2021

Ch3LL commented Oct 8, 2021

myii commented Oct 2, 2021 •

edited

myii left a comment •

edited