-
Notifications
You must be signed in to change notification settings - Fork 46
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
Don't publish recipes that require advanced capabilities to the general collection #1960
Conversation
@jaredkerim @leplatrem Can you both look at this? I think it's broad enough that it would benefit from both of your reviews. I know that Mat is on PTO next week, which is fine. This can just wait a while. |
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.
I couldn't spot any major issue.
Note: We released a «rollback» feature on Remote Settings (yet to be deployed) that will simplify greatly the handling of publishing errors.
normandy/recipes/api/filters.py
Outdated
|
||
if baseline_only: | ||
recipes = list(qs) | ||
assert all(isinstance(recipe, Recipe) for recipe in recipes) |
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.
note: assertions errors are not raised if the server runs with python -O
(optimized)
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.
Good point. I'll change this to an exception.
|
||
requests = requestsmock.request_history | ||
for i, req in enumerate(requests): | ||
print(i, req.method, req.url) |
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.
leftover?
submitted = requestsmock.request_history[4].json() | ||
requests = requestsmock.request_history | ||
for i, req in enumerate(requests): | ||
print(i, req.method, req.url) |
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.
leftover?
# Approving fails. | ||
requestsmock.request("patch", collection_url, status_code=403) | ||
print("expecting patch to:", rs_urls["workspace"]["capabilities"]["collection"]) |
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.
leftover?
…al collection Fixes mozilla#1935
88a651a
to
7ec2944
Compare
I look forward to this! You should file an issue here when that is something we can use here. bors r= @leplatrem |
1960: Don't publish recipes that require advanced capabilities to the general collection r=leplatrem a=mythmon This ended up being way more complicated than I thought. I'm not really happy with the amount of duplication and complexity it has introduced, but I also don't see a great way to improve it. Any ideas? Fixes #1935 Co-authored-by: Mike Cooper <mythmon@gmail.com>
This ended up being way more complicated than I thought. I'm not really happy with the amount of duplication and complexity it has introduced, but I also don't see a great way to improve it. Any ideas?
Fixes #1935