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

Repomanage tests, add srpm to modules, remove redundant packages (RhBug:1804720) #825

Merged
merged 5 commits into from Jun 5, 2020

Conversation

kontura
Copy link
Contributor

@kontura kontura commented May 5, 2020

While trying to have saner repomanage output I have noticed there are several unused packages which only make things more complicated.

There are also various fixes for modular metadata and modular packages.

https://bugzilla.redhat.com/show_bug.cgi?id=1804720

Copy link
Contributor

@pkratoch pkratoch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please also add a scenario for case when there are both modular and non-modular packages of the same name in the repo?

Also, this is not related to the bug, but since you are also adding the basic tests for repomanage, could you also add a scenarios for the case when there are multiple files for the newest nevra and test that repomanage --new will output both files and --repomanage new -k 2 will output both files and the next one? Because this makes repomanage different from just returning the latest packages.


Scenario: basic functionality of repomanage --new works without repodata
Given I copy repository "dnf-ci-thirdparty-updates" for modification
And I delete directory "/{context.dnf.repos[dnf-ci-thirdparty-updates].path}/repodata"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although it works now, the first '/' shouldn't be there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It actually has to be there, this way the path start with // which means it doesn't get prepended with installroot, that is what we need since the {context.dnf.repos[dnf-ci-thirdparty-updates].path} already contains the installroot path.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really dislike this btw. Thinking about it maybe we could prepend installroot only if the path doesn't start with a format placeholder. Because I think it almost always should if it's not an installroot path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, that could work, paths not starting with format placeholder really shouldn't be pointing to the host and thus can be prepended.

But this pr is complex enough, so I don't want to do this kind of change here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, my bad.

dnf-behave-tests/features/plugins-core/repomanage.feature Outdated Show resolved Hide resolved
@kontura
Copy link
Contributor Author

kontura commented Jun 1, 2020

Also, this is not related to the bug, but since you are also adding the basic tests for repomanage, could you also add a scenarios for the case when there are multiple files for the newest nevra and test that repomanage --new will output both files and --repomanage new -k 2 will output both files and the next one? Because this makes repomanage different from just returning the latest packages.

I have added two new tests with duplicate packages, it turned out there was actually a bug in there, good catch. 👍

Can you please also add a scenario for case when there are both modular and non-modular packages of the same name in the repo?

Changed repo to dnf-ci-fedora-modular-updates for Scenario: list packages as new if they are in different stream or are non modular, because it contains a non-modular nodejs as well as several other streams of nodejs.

@pkratoch
Copy link
Contributor

pkratoch commented Jun 5, 2020

I have added two new tests with duplicate packages, it turned out there was actually a bug in there, good catch. +1

Well, now I just hope this indeed was the intended behavior.

Thanks for adding the tests, looks good to me.

@pkratoch pkratoch merged commit c185c7f into rpm-software-management:master Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants