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

Anaconda addon: setup() and execute() no longer get instclass #2092

Merged
merged 1 commit into from
May 31, 2019

Conversation

AdamWill
Copy link
Contributor

Since anaconda commit
rhinstaller/anaconda@78fd1e82 ,
anaconda addons should no longer expect an install class to be
passed to their setup() and execute() methods. Without this fix,
the addon breaks recent versions of anaconda.

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

Signed-off-by: Adam Williamson awilliam@redhat.com

@candlepin-bot
Copy link

Can one of the admins verify this patch?

@AdamWill
Copy link
Contributor Author

Note, this will only work with newer anaconda versions. If you want the code to be compatible with both older and newer anaconda, we'd have to do something a bit uglier, like just making the last two arguments to the methods optional (in theory we should then figure out from how many arguments were passed what the arguments actually were and handle them appropriately, but since both these methods basically do nothing, we probably wouldn't need to bother).

@AdamWill
Copy link
Contributor Author

OK, so I noticed that it seems we use the latest version of subscription-manager for old Fedora releases, so I went ahead and tweaked this to work with both old and new anaconda.

@jirihnidek jirihnidek self-assigned this May 27, 2019
@jirihnidek
Copy link
Contributor

@candlepin-bot ok to test

Copy link
Contributor

@jirihnidek jirihnidek left a comment

Choose a reason for hiding this comment

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

Thanks for PR. It works as expected. Please, can you change comments little bit?

# (storage, ksdata, payload). If this method *did*
# anything we would have to figure out which args we'd got and
# handle them appropriately, but since it does nothing, we
# don't have to bother with that.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not native English speaker, but please, can you change the comment to use little bit better English grammar?

# (storage, ksdata, users, payload). If this method *did*
# anything we would have to figure out which args we'd got and
# handle them appropriately, but since it does nothing, we
# don't have to bother with that.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same request here.

@AdamWill
Copy link
Contributor Author

Um...I am a native English speaker and there's nothing wrong with the grammar. :) If you don't like it for some reason I can rewrite it some other way, though.

Since anaconda commit
rhinstaller/anaconda@78fd1e82 ,
anaconda addons should no longer expect an install class to be
passed to their setup() and execute() methods. Without this fix,
the addon breaks recent versions of anaconda. As the policy
for this tool seems to be that the most recent version is sent
to all Fedora releases (and RHEL releases?), I implemented this
in such a way that it will work with both old and new anaconda.

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

Signed-off-by: Adam Williamson <awilliam@redhat.com>
@jirihnidek
Copy link
Contributor

Um...I am a native English speaker and there's nothing wrong with the grammar. :) If you don't like it for some reason I can rewrite it some other way, though.

Uh, I'm sorry. :-) I'm probably too old and I expect at least uppercase letter at the beginning of the sentence and... Never mind. It's only comment. I'm merging it to master.

Copy link
Contributor

@jirihnidek jirihnidek left a comment

Choose a reason for hiding this comment

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

ACK

@jirihnidek jirihnidek merged commit 6c99f29 into candlepin:master May 31, 2019
@AdamWill
Copy link
Contributor Author

Oh, I see. Yeah, I often start comments with a lower-case character, not sure why really, it often just reads odd to me for them to start with a capital. I guess to me an in-line comment reads like it's somehow part of "the same sentence" as the code it's commenting on, quite often, rather than being a separate thing...

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.

3 participants