pagure

#1647 Rebasing the private_repo branch over master
Merged by pingou. Opened by farhaan.
farhaan/pagure private-repo  into  master

Download 1647.patch
no initial comment

rebased

This PR is not up for review yet :smile:

rebased

rebased

rebased

1 new commit added

  • Fix _get_project to get_authorized project for the new files and removing imports

This pr is open for review and testing now :smile:

rebased

Did you rebase the migration script? (Try running alembic history to see what I mean)

I think we dropped this, no? :)

hm, not quite sure to follow what we are filtering here

Isn't this also depending on pagure's configuration?

Could you just refresh my memory, why the second check here (if username is the current user?)

We may want to add a comment here explaining why that is

There is one thing missing, but I'm willing to accept it in a different PR (assuming that work is started at the time this PR is merged), it's: documentation :)

work in progress :stuck_out_tongue_winking_eye:

for the user to view his private and public project if the second check is not there user will not be able to view his private project AFAIR , because I struggled with it and to be more precise it's your commit ;)

we are filtering public and private project of the specific user.

we are filtering: private == false OR private == true, so what's the third option? :)

Ah, I see. It's the other way around in fact, it's to prevent other users from viewing the user's private projects. Otherwise, I'm authenticated, viewing your page and seeing your private repo.

Maybe we should document this in a comment since we seem to easily forget :)

Thanks for pointing out , this slipped my mind !

s/pivate/private

Should this be moved into pagure.lib? It's using a private method from that package.

The name is a little bit weird since "project" and "repo" seem to be used interchangeably. The database model is Project, I believe, so it would be better to consistently use that. Maybe call the repo parameter "project_name" if that's what it is.

Something like:

Retrieve the project, taking into account user permissions
:param session: The SQLAlchemy session to use
:type  session: sqlalchemy.orm.session.Session
:param project_name: The project name
:type  project_name: str
:param user: A Pagure user
:type  user: str
:param namespace: A Pagure namespace
:type  namespace: str
:return: The project object if the repository is public or if the user is authorized to view the private repository, otherwise None is returned.
:rtype:  pagure.models.Project

I should note that I'm not sure what documentation formatting @pingou prefers. NumPy/Google style doc strings can be handled by the Napoleon sphinx extension and look like:

Args:
    session (Session): The SQLALchemy :class:`Session` to use
    project_name (str): ...
Returns:
    pagure.models.Project: The project object if the repository is public or if the user...

Which many consider to be much more legible

Looking through the PR, it seems like all the uses of pagure.lib.get_project have been replaced with this. Would it make more sense to simply make that function aware of private repos?

At the beginning it was to make sure we're always checking for private repo so we renamed the original method, making sure it would break for everyone except those who really wanted/needed the old method (and were thus adjusted).

However, now that I think about it, we could just have rename the old method _get_project and create this method as get_project in pagure.lib automatically moving everyone to this method.

This should make the change set much smaller as well since only a few method will have to call the old _get_project.

I remember this was something @puiterwijk discussed, so his input might be nice in case I am mis-remembering.

I like the first style more :)

@pingou that was exactly the case, and I think the main reason it was split out as a separate function was because it should be touching the users' login session, to check if they have access, and you didn't want stuff in pagure.lib accessing that, as they should be framework-independent.

That sounds like something I would say indeed :)

Thanks @puiterwijk, I had forgot that aspect

rebased

@pingou @jcline This is up for review :smile:

rebased

rebased

rebased

you don't need to start with a different column. You can simply do nullable=True, then set the value and then do nullable=False.

80 char

80

namespce=namespace

namespace is not specified here

1 new commit added

  • Fix the changes suggested by @vivekanad1101

1 new commit added

  • Fix 80 character pep8 convention

rebased

rebased

rebased

Too much indentation.

Do you need to end with closing colon or not?

It doesn't need to, because this is a repo object, not a repo name.

How about instead of adding this check to tons of places, just have the notify.log check project.private?

I am ending with a colon

rebased

rebased

out of the 4 places you have revision id, at least one is wrong

you probably don't need new_column_name

should we mention that these repositories would be visible to site admins?

[Project] ? when?

i don't know if this line actually crosses 80 char limit but, can you check it once? (since, you will need to change a few other things anyway.)

rebased

92 new commits added

  • Fix tests and migrated to _getrepo
  • Fix test cases and replace get_project after acl
  • Fix private-repo feature after rebase
  • Fix the check for project=None
  • Fix docstring for get_authorized_project
  • Refactor code to have lesser check for sending notification
  • Fix test for private repo
  • Replacing get_project by get_authorized project after rebase
  • Fix test after rebase
  • Fix 80 character pep8 convention
  • Fix the changes suggested by @vivekanad1101
  • Add documetation about private repo
  • Fix tests
  • Document code and fix condition to filter
  • Fix tests after changing parameter name
  • Document the get_authorized_project function and change parameter name
  • Fix alembic version
  • Fix tests after rebase
  • Fix _get_project to get_authorized project for the new files and removing imports
  • Fix test after FORK_FOLDER change
  • Migration from get_project to _get_project
  • Fixed more breaking tests after recent rebase
  • Fix keys and file path for test
  • Fix failing test in ui and repo path fixed for test
  • Fix 401 error because of wrong acls
  • Fix modified json response
  • Fix using get_authorized_project in api/fork
  • Fix test for changing status of an issue
  • Fix test for api response json
  • Change of get_project to _get_project
  • Fix test for private repo in api/issue
  • Fix test for pushing to non-bare repo in private repo
  • Fix get_authorized_project
  • Fix overwriting conf file
  • Disable pagure ci for private repo
  • Fix test after the addition of conf key
  • Add conf key to turn private-repo on and off
  • Fix fedmsg and redis leaking private-repo info
  • Fix _get_project where it is required
  • Fix test for private-repo and rename get_project to _get_project
  • Replace get_project to get_authorized_project
  • Fix test after rebase
  • Fix test failling for card-header count and py3 compatibility
  • Fix docstring for get_authorized_project
  • Fix test for repo and fork ui
  • Fix missing import and pep 8
  • Test for private repo view issue comment api endpoint
  • Add test for adding comment to issue api endpoint
  • Add test for changing status through api point in private repo
  • Add tests to test issues api for private repo
  • Add test for Pr merge and close in private repo
  • Add test to flag PR in private repo
  • Add test private repo api endpoit for adding comment to PR
  • Add test a single PR retrival
  • Add test for pull request api and condition for pull request api(private
  • Add test for api end point to get projects
  • Fix api point to fetch tags
  • Add a different method to fetch project
  • Add is_repo_admin parameter to get_project
  • Fix pep 8 issues and remove duplication
  • Add check for api endpoint for private repo
  • Add check for repo admin when repo is private
  • Add test to check issues and pull-request
  • Add test for issues to private repo and changes to fix those test
  • Fix pep8 conventions
  • Fix duplicate lines
  • Add test for adding user to the private repo
  • Fix test for pull-request for private repo
  • Fix GIT_FOLDER in test for private repo
  • Add change for PR
  • Add test to check private checkbox UI
  • Fix test private repo UI
  • Add Test for UI and Pull request to Private Repo
  • Check if the user authenticated is the one for whom we're viewing the repos
  • Add test for private repo
  • Fix trailing space
  • Fix conditions
  • Fix test delete_repo endpoint
  • Fix typo
  • Fix header
  • Fix parameter
  • Fix variable change
  • Fix variable change and default private
  • Fix UI issuses and authorization issues
  • Fix repo controller
  • Fix repo controller
  • Fix alembic script
  • Add alembic migration and clean code
  • Fix update project and update project UI
  • Fix more conditions
  • Add private repo support
  • Add Private Repo UI

rebased

it can return a list?

I wonder if we shouldn't bail if project is None now

Not sure if this is pep8 valid :s

This is likely needed in other places :)

?

?

same here :)

I don't think this indentation is valid pep8 :)

Don't we want to keep as it is?

I guess we can remove this one

And this one :)

Not sure we want to use get_authorized_project() here

Same here

same here

and here

I'd have put the new line after the opening bracket

here as well (as below)

except errors we normally use 2 spaces indentation in the html

One too many

isn't that already covered ?

We could keep using _get_project() in most of the tests

?

Basically I think it only make sense for the tests working with a private project.

Not sure what to do here

rebased

pretty please pagure-ci rebuild

This is the default hook, so we may want to use _get_project() here.

Also if project is None, is a use-case that is covered in the other hooks, so easy to adjust ;-)
(But this could be done in another PR though)

new_column_name needs to be drop otherwise we're trying to rename private into private which will fail :)

s/user/users/

The name get_authorized_project is confusing me. It sounds like it will return any project, since it's an authorized call...

Shouldn't all hooks use _get_project anyway, since they're run by the project owner during push?
The flask session will probably always be None, so otherwise we'd be breaking all hooks....

And another hook.

93 new commits added

  • Fix pep8 convention and chages suggested by @pingou
  • Fix tests and migrated to _getrepo
  • Fix test cases and replace get_project after acl
  • Fix private-repo feature after rebase
  • Fix the check for project=None
  • Fix docstring for get_authorized_project
  • Refactor code to have lesser check for sending notification
  • Fix test for private repo
  • Replacing get_project by get_authorized project after rebase
  • Fix test after rebase
  • Fix 80 character pep8 convention
  • Fix the changes suggested by @vivekanad1101
  • Add documetation about private repo
  • Fix tests
  • Document code and fix condition to filter
  • Fix tests after changing parameter name
  • Document the get_authorized_project function and change parameter name
  • Fix alembic version
  • Fix tests after rebase
  • Fix _get_project to get_authorized project for the new files and removing imports
  • Fix test after FORK_FOLDER change
  • Migration from get_project to _get_project
  • Fixed more breaking tests after recent rebase
  • Fix keys and file path for test
  • Fix failing test in ui and repo path fixed for test
  • Fix 401 error because of wrong acls
  • Fix modified json response
  • Fix using get_authorized_project in api/fork
  • Fix test for changing status of an issue
  • Fix test for api response json
  • Change of get_project to _get_project
  • Fix test for private repo in api/issue
  • Fix test for pushing to non-bare repo in private repo
  • Fix get_authorized_project
  • Fix overwriting conf file
  • Disable pagure ci for private repo
  • Fix test after the addition of conf key
  • Add conf key to turn private-repo on and off
  • Fix fedmsg and redis leaking private-repo info
  • Fix _get_project where it is required
  • Fix test for private-repo and rename get_project to _get_project
  • Replace get_project to get_authorized_project
  • Fix test after rebase
  • Fix test failling for card-header count and py3 compatibility
  • Fix docstring for get_authorized_project
  • Fix test for repo and fork ui
  • Fix missing import and pep 8
  • Test for private repo view issue comment api endpoint
  • Add test for adding comment to issue api endpoint
  • Add test for changing status through api point in private repo
  • Add tests to test issues api for private repo
  • Add test for Pr merge and close in private repo
  • Add test to flag PR in private repo
  • Add test private repo api endpoit for adding comment to PR
  • Add test a single PR retrival
  • Add test for pull request api and condition for pull request api(private
  • Add test for api end point to get projects
  • Fix api point to fetch tags
  • Add a different method to fetch project
  • Add is_repo_admin parameter to get_project
  • Fix pep 8 issues and remove duplication
  • Add check for api endpoint for private repo
  • Add check for repo admin when repo is private
  • Add test to check issues and pull-request
  • Add test for issues to private repo and changes to fix those test
  • Fix pep8 conventions
  • Fix duplicate lines
  • Add test for adding user to the private repo
  • Fix test for pull-request for private repo
  • Fix GIT_FOLDER in test for private repo
  • Add change for PR
  • Add test to check private checkbox UI
  • Fix test private repo UI
  • Add Test for UI and Pull request to Private Repo
  • Check if the user authenticated is the one for whom we're viewing the repos
  • Add test for private repo
  • Fix trailing space
  • Fix conditions
  • Fix test delete_repo endpoint
  • Fix typo
  • Fix header
  • Fix parameter
  • Fix variable change
  • Fix variable change and default private
  • Fix UI issuses and authorization issues
  • Fix repo controller
  • Fix repo controller
  • Fix alembic script
  • Add alembic migration and clean code
  • Fix update project and update project UI
  • Fix more conditions
  • Add private repo support
  • Add Private Repo UI

Someone just pointed out that Pagure private issues email mentioned people even in private tickets, and I think this might be the same in private projects.
We should make sure that that doesn't happen :).

@puiterwijk for tickets this was very much in purpose, the bit that is missing is to allow people that are mentioned to access the ticket.

rebased

rebased

Ok tests are passing, it has been rebased and @puiterwijk has agreed that we work on namespacing the private repo in a different PR.

So let's merge this one :)

Pull-Request has been merged by pingou

Thanks @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.