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

Add coverage for improved countme system age #1501

Merged
merged 8 commits into from
Aug 29, 2024

Conversation

dmnks
Copy link
Contributor

@dmnks dmnks commented May 9, 2024

This updates the countme feature file in accordance with rpm-software-management/libdnf#1662.

dmnks added 4 commits May 9, 2024 10:17
Make sure *no* countme flag is present.
Also adapt the HTTP matching patterns in the countme feature file
accordingly.

This will be used in the next commit, no functional change right now.
Shuffle some steps around, add blank lines and also emphasize that we're
dealing with *calendar* weeks/months (i.e. aligned with the calendar,
not with the first countme hit).

No functional change.
We'll need this step in a "when" context in the next commit.

No functional change.
@pep8speaks
Copy link

pep8speaks commented May 9, 2024

Hello @dmnks! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-08-07 07:54:10 UTC

@ppisar
Copy link
Contributor

ppisar commented May 10, 2024

I think we also need to add the machine-id code into dnf5.

@dmnks
Copy link
Contributor Author

dmnks commented May 10, 2024

Yup, porting this to dnf5 was the intention, indeed. I made the patch in dnf4 just because that's what I was most familiar with.

It's probably best to first let the dnf4 patch go through the usual review process and only then create a dnf5 version of it, to keep things more manageable. I can help with the latter too (or just do it myself, the related code is basically the same so it shouldn't be too hard).

@mattdm
Copy link

mattdm commented May 17, 2024

It's probably best to first let the dnf4 patch go through the usual review process and only then create a dnf5 version of it, to keep things more manageable. I can help with the latter too (or just do it myself, the related code is basically the same so it shouldn't be too hard).

Can we make sure that the change only lands in rawhide and f41+? That way, I'll know where/when the change happened and can take that into account in interpreting the data.

@jan-kolarik
Copy link
Member

It's probably best to first let the dnf4 patch go through the usual review process and only then create a dnf5 version of it, to keep things more manageable. I can help with the latter too (or just do it myself, the related code is basically the same so it shouldn't be too hard).

Can we make sure that the change only lands in rawhide and f41+? That way, I'll know where/when the change happened and can take that into account in interpreting the data.

Yep, I'll sync about that with our colleague doing releases.

@dmnks
Copy link
Contributor Author

dmnks commented May 20, 2024

@jan-kolarik is dnf4 shipped in f41/rawhide at all? I'm thinking if it makes sense to make this change in dnf4 and if we shouldn't just skip dnf4 entirely.

@jan-kolarik
Copy link
Member

jan-kolarik commented May 20, 2024

@jan-kolarik is dnf4 shipped in f41/rawhide at all? I'm thinking if it makes sense to make this change in dnf4 and if we shouldn't just skip dnf4 entirely.

Ehm, good point 😄 Well, basically it is not, as the default is already the dnf command from dnf5 package. So this would only affect users explicitly using dnf-3 binary (or dnf4 symlink). And probably also libdnf API users...

@dmnks
Copy link
Contributor Author

dmnks commented May 22, 2024

OK, therefore the dnf4 patch still makes sense to pursue, I suppose.

I wonder, however, if we shouldn't apply the patch conditionally (in both the libdnf4 and libdnf5 specs) only on F41+ then?

Having a dnf config option to choose whether the age reported should be absolute (based on the machine-id timestamp) or relative (based on the first countme flag sent for the given repo) would be another option. It would make it easier/cleaner to control on which distros this is enabled, although it might as well be overkill, so I'm not sure.

WDYT?

@jan-kolarik
Copy link
Member

@dmnks Sorry, I somehow missed your comment.

OK, therefore the dnf4 patch still makes sense to pursue, I suppose.

I think so. If we then port it to dnf5, the work should be easier, and we'll cover all dnf clients in Rawhide, as some may use dnf5 and others dnf4 in F41.

I wonder, however, if we shouldn't apply the patch conditionally (in both the libdnf4 and libdnf5 specs) only on F41+ then?

We usually try to keep the latest Fedora and upstream in sync, so I'd prefer to apply the revert patch for F40 and F39.

If no one else takes it, I'd like to review your PR in the coming days. Thanks!

@dmnks
Copy link
Contributor Author

dmnks commented May 29, 2024

@dmnks Sorry, I somehow missed your comment.

No worries.

I think so. If we then port it to dnf5, the work should be easier, and we'll cover all dnf clients in Rawhide, as some may use dnf5 and others dnf4 in F41.

Ack!

We usually try to keep the latest Fedora and upstream in sync, so I'd prefer to apply the revert patch for F40 and F39.

Ack, whatever works for you best 👍

If no one else takes it, I'd like to review your PR in the coming days. Thanks!

Sounds like a plan, thank you! If/when you get around to it, feel free to ping me on Slack if you need any help.

@jan-kolarik
Copy link
Member

I think we also need to add the machine-id code into dnf5.

I've created the dnf5 counterpart for this: rpm-software-management/dnf5#1525.

@jan-kolarik
Copy link
Member

/packit test rpm-software-management/libdnf#1662

@jan-kolarik
Copy link
Member

@dmnks Oh, I just noticed that now - this needs to be targeted against dnf-4-stack as the main branch is for dnf5 stuff...

@dmnks
Copy link
Contributor Author

dmnks commented Jun 3, 2024

Oh... Thanks. I should open a new PR against dnf-4-stack, correct?

@jan-kolarik
Copy link
Member

Oh... Thanks. I should open a new PR against dnf-4-stack, correct?

Probably yes, I don't know if you can switch the target branch of the existing PR.

@dmnks
Copy link
Contributor Author

dmnks commented Jun 3, 2024

Ported: #1512

Adapt the countme feature to the libdnf fix for issue #1611, namely:

- Turn the main scenario into a scenario outline to capture the various
  machine-id(5) configurations (see the table).

- Add an upgrade scenario (from F39 to F40) that verifies that system
  age is now independent of $releasever on systems with a machine-id
  file.

- Use NO_FAKE_STAT=1 in faketime invocations so that filesystem
  timestamps are *not* reported relative to the target time (this would
  break our custom machine-id timestamps we set here), see faketime(1)
  for details.

- Add a new MachineId class to encapsulate the machine-id file, similar
  to OSRelease.

- Mark the touched scenarios as destructive (due to them overriding the
  machine-id file).
There is a difference between `makecache` command from dnf5 and old
dnf4. dnf4 always fetches new metalink/mirrorlist and ignores expiration
of the repo, this is likely a long-standing bug. On the other hand dnf5
doesn't contant the remote if the cache is fresh.

Since the countme tests rely on the dnf4 behavior ensure it by using
`--refresh` for dnf5.
According to OS-RELEASE(5) man page values with characters outside of
A–Z, a–z, 0–9 such as spaces or dots must be enclosed in double/single
quotes.

While handling of the `os-release` file is still work in progress in
dnf5 make the step more robust by wrapping all values in quotes.

Old dnf4 was able to handle even non-conforming `os-release` file.
@kontura
Copy link
Contributor

kontura commented Aug 7, 2024

I have added a couple more commits to enable the tests for dnf5.
The tests still require both: rpm-software-management/dnf5#1613 and rpm-software-management/dnf5#1590.

@jan-kolarik
Copy link
Member

@dmnks and @kontura Thanks for making this functionality extensively covered with tests guys!

@jan-kolarik jan-kolarik merged commit 5157f49 into rpm-software-management:main Aug 29, 2024
5 checks passed
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.

6 participants