-
Notifications
You must be signed in to change notification settings - Fork 38
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
Fixes #131 #132
Fixes #131 #132
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @evil-g !
This looks good, just a few notes. 👍🏼
Maybe you could add a test case for this PR ? Or I'll add that later :)
sure no worries, i'll add a test as well and update the PR 👍 |
hey @davidlatwe , sorry for the delay on that. i pushed the cleanup stuff and a first pass at the new unit test. |
Thanks for the quick move :D |
Well, the last running test seems to be stuck at job initializing, it holds there for over 10 mins. Let's call it all passed ! |
if package_filter.excludes(app_package): | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just leaving notes:
While I was resolving merge conflict, I found #129 actually also accidentally fixed the issue that this PR is aiming to. In #129, instead of using _rez_api.find_latest
, it has changed to use contorller's find
method which already filtering packages with filter.
Still, removing this line here is good, it's redundant anyway.
oh yeah you're right...looks like it used the credentials for our internal gitlab. i'll update my git config for next time. |
This should fix the bug in #131.
There's a handy iter_packages method on the rez package filter object, so just updated the existing utils to take a package_filter arg.
(Updated find_one too for posterity)