pagure

#1274 Set the most common variables to the request scope
Merged by pingou. Opened by pingou.
decorator_repo  into  master

Download 1274.patch
no initial comment

When would repo not be set? Could you add a comment here explaining when that would happen for future readers?

I'm a little newer to flask. Is setting things like this flask.g a common convention? Does this basically just add objects to this request? If so, why not tack on to flask.request? Finally, does setting objects on flask.g only affect the current request being handled, and not other simultaneous requests (i.e., is this thread safe?)

It seems like it could be confusing to future readers to overwrite the repo variable here, since it is a parameter in the top of the function. I see that you are setting repo to reponame at the top of the function. Perhaps you could call this repo_object, or just use flask.g.repo everywhere instead so it's not so confusing?

The above is just a suggestion, feel free to ignore.

Why would this change from 403 to 404?

This looks great, so much code de-duplication!

1 new commit added

  • Explain a little more what's happening in the @repo_method

LGTM!

I didn't want to use repo_obj as I already use it everywhere for the pygit2 repository object, but I'll adjust to use plain flask.g.repo, thanks :)

Because the existence of the git repo is now done prior to the check if the user is allowed to do the action.

I should move up the creation of the git so that we can still also check the 403.

Thanks

7 new commits added

  • Also check that if the user is not allowed, they get a 403 error
  • Some more relying on flask.g and some code adjustments
  • Adjust unit-tests for the new @app.before_request method
  • Add a small new check for the http code returned
  • Adjust the test so that we only drop the @app.before_request from flask-fas-openid
  • Drop the @repo_method decorator
  • Drop the @repo_method decorator and use an @app.before_request instead

3 new commits added

  • Some more cleaning and relying on flask.g
  • Adjust the templates to rely on the variables in flask.g
  • Port the controllers remaining to rely on the variables in flask.g

rebased

1 new commit added

  • Adjust unit-tests for the change in check of the git repo

1 new commit added

  • Another attempt to fix running the tests on jenkins

:thumbsup:

Thanks for the review @vivekanand1101

I'll wait a bit before merging in case @bowlofeggs wants to have a look at the final changes since he did the first set :)

s/retrieve/retrieves/

s/provide/provides/

Maybe add a comment here explaining why this is happening?

This seems like a really nice approach since it doesn't require so many decorators. Even more code de-duplication!

Nice work @pingou, LGTM!

1 new commit added

  • Adjust docstring and comment as per @bowlofeggs suggestion

:thumbsup:

Thanks for the review @vivekanand1101 and @bowlofeggs, it wasn't a small one :)

Pull-Request has been merged by pingou
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.