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

Fix normalization of hardware key/option #3015

Merged
merged 1 commit into from
Jun 17, 2024
Merged

Conversation

happz
Copy link
Collaborator

@happz happz commented Jun 13, 2024

A bug was introduced in the patching adding multiple disks to testcloud plugin, and the test was missing.

Pull Request Checklist

  • implement the feature

A bug was introduced in the patching adding multiple disks to testcloud
plugin, and the test was missing.
@happz happz added bug Something isn't working area | hardware Implementation of hardware requirements labels Jun 13, 2024
@happz happz added this to the 1.34 milestone Jun 13, 2024
@happz happz added the ci | full test Pull request is ready for the full test execution label Jun 13, 2024
@happz
Copy link
Collaborator Author

happz commented Jun 13, 2024

@martinhoyer to my horror, I broke hardware normalization, and since I didn't bother to add a unit test, nobody noticed till I used --hardware just this evening. So, I suppose one more to include in 1.34, this time it's a genuine bug fix :/

@martinhoyer
Copy link
Collaborator

Nice catch. Thanks for the heads-up.
I'd say we can do the release on Monday anyway. (DevConf)
Unless you really want to do this week :)

Copy link
Collaborator

@martinhoyer martinhoyer left a comment

Choose a reason for hiding this comment

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

It looks good to me, but clearly I don't understand the code enough to be able to click approve with clear conscience.
@happz If you could help me understand, it would be much appreciated 🙏
I get that line 519 is merged[components.name] = [] as there is peer_index, but am not sure why merged[components.name] is otherwise {}. Am I looking correctly at

name=groups['name'],
?

@martinhoyer
Copy link
Collaborator

@psss @lukaszachy Could you take a look please, so we can do the release? :)

@lukaszachy
Copy link
Collaborator

lukaszachy commented Jun 17, 2024

Hm, there is missing fmf on rawhide (as it cannot be built https://kojipkgs.fedoraproject.org//work/tasks/6207/118796207/build.log) which blocks tmt.

Hm, we should monitor bugzilla as well, it was opened in May :/ (https://bugzilla.redhat.com/show_bug.cgi?id=2279994)

@martinhoyer
Copy link
Collaborator

Hm, there is missing fmf on rawhide (as it cannot be built https://kojipkgs.fedoraproject.org//work/tasks/6207/118796207/build.log) which blocks tmt.

Hm, we should monitor bugzilla as well, it was opened in May :/ (https://bugzilla.redhat.com/show_bug.cgi?id=2279994)

whoops. I wonder if teemtee/fmf#194 has something to do with it.
Probably shouldn't block this PR though?

@martinhoyer
Copy link
Collaborator

martinhoyer commented Jun 17, 2024

whoops. I wonder if teemtee/fmf#194 has something to do with it. Probably shouldn't block this PR though?

actually, I think it's the use of pkg_resources(I'll stop guessing). Looking into it.

@happz
Copy link
Collaborator Author

happz commented Jun 17, 2024

It looks good to me, but clearly I don't understand the code enough to be able to click approve with clear conscience. @happz If you could help me understand, it would be much appreciated 🙏 I get that line 519 is merged[components.name] = [] as there is peer_index, but am not sure why merged[components.name] is otherwise {}. Am I looking correctly at

name=groups['name'],

?

merged is basically this structure:

merged:
  # merged[component.name] = [] for these:
  disk:
    - size: 10GB
    - size: 10GB
  network:
    - type: eth

  # merged[component.name] = {} for these:
  cpu:
    flags:
      - avs
      - avs2
    model: 79
  device:
    vendor: ...

  # and merged[component.name] is whatever the value is for these:
  hostname: foo.bar.com
  memory: 4GB

The issue was in merged[components.name] = [] being used for all three kinds of requirements.

Copy link
Collaborator

@martinhoyer martinhoyer left a comment

Choose a reason for hiding this comment

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

Thanks @happz, much appreciated!

@happz happz merged commit 87f2fe0 into main Jun 17, 2024
16 of 19 checks passed
@happz happz deleted the fix-hw-normalize-merge branch June 17, 2024 13:04
The-Mule pushed a commit to The-Mule/tmt that referenced this pull request Oct 14, 2024
A bug was introduced in the patching adding multiple disks to testcloud
plugin, and the test was missing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area | hardware Implementation of hardware requirements bug Something isn't working ci | full test Pull request is ready for the full test execution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants