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

Fixes #131 #132

Merged
merged 12 commits into from
Nov 18, 2020
Merged

Fixes #131 #132

merged 12 commits into from
Nov 18, 2020

Conversation

evil-g
Copy link

@evil-g evil-g commented Nov 17, 2020

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)

Copy link
Collaborator

@davidlatwe davidlatwe left a 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 :)

allzpark/control.py Outdated Show resolved Hide resolved
allzpark/_rezapi.py Outdated Show resolved Hide resolved
@evil-g
Copy link
Author

evil-g commented Nov 17, 2020

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 👍

@davidlatwe
Copy link
Collaborator

Hi @evil-g,
I may merging #129 later today, but that would produce conflict to this PR, so I'll resolve it and update this PR. If that woks for you ?

@evil-g
Copy link
Author

evil-g commented Nov 18, 2020

hey @davidlatwe , sorry for the delay on that. i pushed the cleanup stuff and a first pass at the new unit test.
but if you end up needing to change something else to merge #129 have at it :)

tests/test_apps.py Outdated Show resolved Hide resolved
@davidlatwe
Copy link
Collaborator

Thanks for the quick move :D
Appreciate it 👍🏼

@davidlatwe
Copy link
Collaborator

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 !
Thanks again @evil-g, will merge this later 🎉

Comment on lines -1168 to -1169
if package_filter.excludes(app_package):
continue
Copy link
Collaborator

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.

@davidlatwe davidlatwe merged commit 0dff686 into mottosso:master Nov 18, 2020
@davidlatwe
Copy link
Collaborator

And 1.3.129 released !
p.s. @evil-g, I found that the commits you made seems not using the account that GitHub recognized so the contributor page doesn't have your name on it, and looks like there's nothing we could do now. But I tagged you in the release note though. :)

@evil-g
Copy link
Author

evil-g commented Nov 19, 2020

And 1.3.129 released !
p.s. @evil-g, I found that the commits you made seems not using the account that GitHub recognized so the contributor page doesn't have your name on it, and looks like there's nothing we could do now. But I tagged you in the release note though. :)

oh yeah you're right...looks like it used the credentials for our internal gitlab. i'll update my git config for next time.
thanks for pointing that out! :)

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