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 the combo box for an URL type of additional repositories (#1879127) #2857

Merged
merged 1 commit into from Sep 17, 2020

Conversation

poncovka
Copy link
Contributor

@poncovka poncovka commented Sep 15, 2020

In the commit ff9a7e1, we introduced new constants for the URL source types with
values BASEURL, MIRRORLIST and METALINK. In the commit cc2c3d3, we started
to use these constants in the Installation Source spoke and removed the old ones
with values url, mirrorlist and metalink. We updated the combo box for the
base repository, but forgot to update the combo box for additional repositories.

The combo box items have to have ids that match the values of the constants,
otherwise the URL type of additional repositories will be always BASEURL.

Resolves: rhbz#1879127

@poncovka poncovka added master Please, use the `f39` label instead. manual testing required This issue can't be merged without manual testing f33 and removed manual testing required This issue can't be merged without manual testing labels Sep 15, 2020
Copy link
Contributor

@VladimirSlavik VladimirSlavik left a comment

Choose a reason for hiding this comment

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

Not sure what to say to this kind of diff. LGTM?

@AdamWill
Copy link
Contributor

So, hum, I'm thinking the commits that changed stuff here were ff9a7e1 and cc2c3d3 ? That latter one seems to have changed one place in installation_source.glade in the same way as this PR, but not the place this PR touches...

so on the face of it this seems like it's probably going in the right direction, but...it'd be nice to know that someone had checked that everything is now consistent and there isn't anything else still looking for the 'old' strings...

@VladimirSlavik
Copy link
Contributor

@AdamWill if you want to do more digging yourself: Without any more info, I'd hazard a guess that somewhere we used to read id of currently selected item in GUI, and fed that to ...method in the kickstart data tree, directly or indirectly. Or compared with possible values thereof. That would imply that now that we have abolished during modularization the Cthulhu of method as source of truth, the new code no longer matched the ids to the right stuff. We now generally go UPPERCASE on constants while method was from the age of kickstart so camelCase or lowercase. This commit/PR message and diff hints at all this, I think.

So maybe try seeing where __init__ or initialize puts the elements changed in glade (or parent element / dropdown), and trace usages of that for reading the id or some such. That would then be the code where the constants no longer matched.

Myself, I'm content with the hypothesis, knowing Vendy wrote a lot of the implied changed code, and seeing the manual testing label dropped. I also think it's not worth chasing if she can just describe the thing for us...

In the commit ff9a7e1, we introduced new constants for the URL source types with
values 'BASEURL', 'MIRRORLIST' and 'METALINK'. In the commit cc2c3d3, we started
to use these constants in the Installation Source spoke and removed the old ones
with values 'url', 'mirrorlist' and 'metalink'. We updated the combo box for the
base repository, but forgot to update the combo box for additional repositories.

The combo box items have to have ids that match the values of the constants,
otherwise the URL type of additional repositories will be always 'BASEURL'.

Resolves: rhbz#1879127
@poncovka poncovka changed the title Fix the UI support for additional repositories with metalinks (#1879127) Fix the combo box for an URL type of additional repositories (#1879127) Sep 16, 2020
@poncovka
Copy link
Contributor Author

poncovka commented Sep 16, 2020

So, hum, I'm thinking the commits that changed stuff here were ff9a7e1 and cc2c3d3 ? That latter one seems to have changed one place in installation_source.glade in the same way as this PR, but not the place this PR touches...

That is correct. I have updated the commit message to make it easier to understand the proposed changes.

so on the face of it this seems like it's probably going in the right direction, but...it'd be nice to know that someone had checked that everything is now consistent and there isn't anything else still looking for the 'old' strings...

I have searched for the old strings and the code seems to be fine now.

@poncovka
Copy link
Contributor Author

@AdamWill if you want to do more digging yourself: Without any more info, I'd hazard a guess that somewhere we used to read id of currently selected item in GUI, and fed that to ...method in the kickstart data tree, directly or indirectly. Or compared with possible values thereof. That would imply that now that we have abolished during modularization the Cthulhu of method as source of truth, the new code no longer matched the ids to the right stuff. We now generally go UPPERCASE on constants while method was from the age of kickstart so camelCase or lowercase. This commit/PR message and diff hints at all this, I think.

This bug is not related to the method issue. The support for additional repositories was barely touched during the transition of payload sources. This bug was caused by replacing one set of constants with another.

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

LGTM!

@poncovka poncovka merged commit 80805c4 into rhinstaller:f33-devel Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f33 master Please, use the `f39` label instead.
4 participants