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

add ability to interact with fasjson #310

Merged
merged 4 commits into from
Jan 26, 2021

Conversation

StephenCoady
Copy link

This commit adds the ability to interact with the new fasjson API. It builds its cache depending on the fasjson config flag from either FAS or the new fasjson.

Signed-off-by: Stephen Coady [email protected]

fmn/fmn_fasshim.py Outdated Show resolved Hide resolved
fmn/fmn_fasshim.py Outdated Show resolved Hide resolved
fmn/fmn_fasshim.py Outdated Show resolved Hide resolved
fmn/fmn_fasshim.py Outdated Show resolved Hide resolved
fmn/fmn_fasshim.py Outdated Show resolved Hide resolved
fmn/fmn_fasshim.py Outdated Show resolved Hide resolved
fmn/fmn_fasshim.py Outdated Show resolved Hide resolved
fmn/fmn_fasshim.py Outdated Show resolved Hide resolved
fmn/fmn_fasshim.py Outdated Show resolved Hide resolved
fmn/config.py Outdated Show resolved Hide resolved
@StephenCoady
Copy link
Author

So I'm not sure what the best way forward here is. The CI is failing and to make it pass there is a lot of work.

The first blocker is removing python 2 support. I'm not sure of the implications of doing that. I think things like that might be best sorted in another PR. At the moment I can't run the tests locally, there are lots of errors even on py36.

Copy link
Member

@abompard abompard left a comment

Choose a reason for hiding this comment

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

I haven't tested it yet, here are a few comments / changes to make.

fmn/fasjson_client.py Outdated Show resolved Hide resolved
fmn/fasjson_client.py Outdated Show resolved Hide resolved
fmn/fasjson_client.py Outdated Show resolved Hide resolved
fmn/fmn_fasshim.py Outdated Show resolved Hide resolved
fmn/util.py Show resolved Hide resolved
fmn/util.py Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
fmn/consumer.py Outdated
@@ -163,7 +164,11 @@ def work(self, session, raw_msg):
log.info("Autocreating account for %r" % username)
openid = '%s.id.fedoraproject.org' % username
openid_url = 'https://%s.id.fedoraproject.org' % username
email = get_fas_email(config.app_conf, username)
fasjson = config.app_conf['fasjson'].get('active')
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if we can assume that fasjson is always there, if we can't we could do something like app_conf.get("fas_json", {}).get("active")

Copy link
Member

Choose a reason for hiding this comment

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

+1 to that

@pypingou
Copy link
Member

Still failing on requests_gssapi

@codecov
Copy link

codecov bot commented Jan 13, 2021

Codecov Report

Merging #310 (d3fb63d) into develop (6adb6fd) will decrease coverage by 0.72%.
The diff coverage is 24.80%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #310      +/-   ##
===========================================
- Coverage    58.33%   57.61%   -0.73%     
===========================================
  Files           72       73       +1     
  Lines         3910     3994      +84     
  Branches       509      519      +10     
===========================================
+ Hits          2281     2301      +20     
- Misses        1559     1621      +62     
- Partials        70       72       +2     
Impacted Files Coverage Δ
fmn/config.py 100.00% <ø> (ø)
fmn/tasks.py 78.78% <ø> (ø)
fmn/fmn_fasshim.py 20.00% <11.11%> (-2.73%) ⬇️
fmn/util.py 27.27% <20.00%> (-3.17%) ⬇️
fmn/fasjson_client.py 35.29% <35.29%> (ø)
fmn/consumer.py 63.63% <50.00%> (-1.07%) ⬇️
fmn/rules/utils.py 86.25% <83.33%> (ø)
fmn/formatters.py 82.14% <100.00%> (ø)
fmn/lib/defaults.py 91.48% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6adb6fd...d3fb63d. Read the comment docs.

Copy link
Member

@abompard abompard left a comment

Choose a reason for hiding this comment

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

Yep, looks good. I commented on a possible minor improvement, but I think it's good to go as is.

return
global client
try:
_add_to_cache(list(client.list_all_entities("users")))
Copy link
Member

Choose a reason for hiding this comment

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

From a memory point of view it would be better to loop over list_all_entities than building the list in memory. Something like:

for users in client.list_all_entities("users"):
    _add_to_cache(users)

But that's not a blocker.

@StephenCoady StephenCoady merged commit 0c8e5ef into fedora-infra:develop Jan 26, 2021
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