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

added RURI-pattern matching and priority #152

Closed
wants to merge 1 commit into from
Closed

added RURI-pattern matching and priority #152

wants to merge 1 commit into from

Conversation

eschmidbauer
Copy link
Contributor

Hello,
This pull request adds 2 features.
--Prioritize the trusted records (highest to lowest priority)
--Match from-uri AND request-uri, if either is blank only one is matched.
Please review the pull request and let me know if any other changes are needed.
Thank you.
Emmanuel

@eschmidbauer
Copy link
Contributor Author

Hello,
Any chance this can be reviewed before the feature request window closes?
Thanks,
Emmanuel

@miconda
Copy link
Member

miconda commented Jun 9, 2015

Master branch will be frozen again after at least 6 months, plenty of time to get new features in v4.4.

The patch got big because you changed the order of existing columns, not adding new column as last one, but inserting in between, so it was not trivial to analyze quickly when I tried a while ago as I found a bit of spare time.

Most likely I can allocate some time next week, as v4.3 will be out this week.

@eschmidbauer
Copy link
Contributor Author

Thank you for the response.
Yes, I did re-order the fields to make it more comprehensive in the long run.
I've been running this patch on my test machine for several weeks without any issues.
I hope you will consider reviewing and testing soon as we want to put this in production.
Thanks,
Emmanuel

@miconda
Copy link
Member

miconda commented Jun 11, 2015

Some comments:

  • loading ordered on priority is using an SQL extension, making the module to fail when used with db_text, db_mongodb or other no-sql backends. This has to be reworked to get rid of the ordering extension specific to sql or made a configuration file option (module parameter)
  • is it a reason to restrict the matching only to r-uri parts of user@domain? May be useful to match also on transport or other parameters. I would live the matching against the full value of the r-uri inside the code. In the config one can set the $ru to whatever it likes to match
  • the code around the printing of sort_order is looking weird, as you add sort_order.len (which is set before to 0) to the buffer -- it is harmless but doesn't look good when someone is checking the code

@miconda
Copy link
Member

miconda commented Jun 16, 2015

Add the changes to the db schema -- you have to update the xml file in lib/srdb1/schema/.

@miconda
Copy link
Member

miconda commented Jun 16, 2015

And extend the documentation of the module with the appropriate description and new parameters -- changes have to be done to xml files in modules/permissions/doc/

@eschmidbauer
Copy link
Contributor Author

Hello,
made changes as you requested.
Please see latest commit.
Thanks,
Emmanuel

@miconda
Copy link
Member

miconda commented Jun 16, 2015

The commit message is not relevant for the all changes done by the patch, but reflects the last operation you did as per comments in this discussion. It is not suitable for the log of source code changes history.

I will apply the patch manually this time if all ok with the review. But for the future be sure you follow the guidelines for commit messages from:

Note also that you can have many commits in the same pull request. No need to rework a new patch always, you can add another patch to complete the changes done with previous patches.

@eschmidbauer
Copy link
Contributor Author

Hello,
You can download the full git diff (it applies clean to master branch) here http://paste.debian.net/download/231826
Thank you.
Emmanuel

@miconda
Copy link
Member

miconda commented Jun 17, 2015

I applied the patch manually, split in few commits, per components.

The diff for each commit is available also on github, at:

Add .diff to the commit path.

I fixed the upgrade of table version for trusted.

You have to make a new pull request to update the docs regarding the priority, to specify what kind of value represents higher priority.

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.

None yet

2 participants