#157 Refactor load_kojisession
Merged by cqi. Opened by cqi.
cqi/rpkg refactor-load_kojisession  into  master

Download 157.patch

Fix #107

Signed-off-by: Chenxiong Qi cqi@redhat.com

The default behaviour of this method is changed (from anon=False to login=True). Fedpkg changes this method, so it would need to be updated. Why not just keep the default same (and the argument name as well)?

Generally this looks good to me.

There is no longer any _anon_kojisession attribute. It is still defined in __init__ method though and there is one test in rhpkg that references it (which can be updated).

Was there any case when both anonymous and logged in session would be needed?

This patch should not change the original behavior. This must be fixed.

Why not just keep the default same (and the argument name as well)?

I found whether to login is the difference in this method, login would be more straightforward then anon. I think I lost sight of fedpkg when making this change. Keeping anon is a better choice. I'll fix this.

Was there any case when both anonymous and logged in session would be needed?

I see anonymouse session is used in mock_config and gitbuildhash.

So if the anon_kojisession is still needed, should the code here assign some value to it?

Oh, I checked the code and didn't find a case that uses both anon_kojisession and kojisession. But, keeping those two should be safe and probably useful for some cases that needs them.

rebased

rebased

Changes in latest commit

  • original behavior is restored
  • recover _anon_kojisession for property anon_kojisession

Ok, looks good to me. I have no more comments.

Pull-Request has been merged by cqi

Metadata