-
Notifications
You must be signed in to change notification settings - Fork 385
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
Configurable run limit on products. #1415
Conversation
Please extensively check this patch for backwards compatibility. I'm not sure how Thrift handles the expansion of structs. What happens if older client tries to send the configuration struct? |
@whisperity We did this kind of expansion for a couple of times (e.g.: #1166, #968). I've also checked this server version with an older client and it works fine (add/remove/list products from command line). |
@csordasmarton Alright then. Was a first glance exclamation, I didn't recall these improvements. |
api/v6/products.thrift
Outdated
@@ -40,6 +41,7 @@ struct Product { | |||
6: bool accessible, // Indicates whether the current user can access this product. | |||
7: bool administrating // Indicates that the current user can administrate the product. |
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.
Comma missing
api/v6/products.thrift
Outdated
@@ -40,6 +41,7 @@ struct Product { | |||
6: bool accessible, // Indicates whether the current user can access this product. | |||
7: bool administrating // Indicates that the current user can administrate the product. | |||
8: shared.DBStatus databaseStatus // Indicates the database backend status. |
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.
Comma missing
api/v6/products.thrift
Outdated
@@ -40,6 +41,7 @@ struct Product { | |||
6: bool accessible, // Indicates whether the current user can access this product. | |||
7: bool administrating // Indicates that the current user can administrate the product. | |||
8: shared.DBStatus databaseStatus // Indicates the database backend status. | |||
9: i64 runLimit // Number of allowed runs for this product. |
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.
Most of the other structs have ,
after the last element so adding a new member does not make the previous line modified.
api/v6/products.thrift
Outdated
@@ -25,7 +25,8 @@ struct ProductConfiguration { | |||
2: string endpoint, | |||
3: string displayedName_b64, | |||
4: string description_b64, | |||
5: optional DatabaseConnection connection | |||
5: optional DatabaseConnection connection, | |||
6: i64 runLimit |
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.
Most of the other structs have ,
after the last element so adding a new member does not make the previous line modified.
|
||
|
||
def upgrade(): | ||
# ### commands auto generated by Alembic - please adjust! ### |
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.
Once the autogenerated operations had been revised to work, these autogenerated comments should be removed.
@@ -69,7 +69,7 @@ function (declare, domAttr, domClass, domConstruct, Dialog, Button, | |||
0 : ['submit'], | |||
/* PRODUCT_ADMIN */ 1 : ['name', 'description'], | |||
/* SUPERUSER */ 2 : ['endpoint', 'dbengine', 'dbhost', 'dbport', | |||
'dbuser', 'dbpass', 'dbname'] | |||
'dbuser', 'dbpass', 'dbname', 'runlimit'] |
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.
#1410 states that product admin
s should be able to change this variable too, not just superusers.
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.
@whisperity If I remember it correctly we discussed that only the superuser should have the right to change this.
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.
Sure. 😉 I don't think I was there on that discussion. But it sounds good that it's a superuser thing.
@@ -267,6 +280,7 @@ function (declare, domAttr, domClass, domConstruct, Dialog, Button, | |||
|
|||
var product = new CC_PROD_OBJECTS.ProductConfiguration({ | |||
endpoint: args['endpoint'], | |||
runLimit: args['runlimit'] ? args['runlimit'] : -1, |
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.
How does -1
harmonise with the logic behind this field? Can't we set this to null
and send a None
into Python?
8fc69ef
to
d00957d
Compare
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 think there is some problem modifying the max run count on the UI.
I started a simple default server (no separate superuser or admin)
If I click to save the max run count I get an error
[INFO 2018-03-05 09:29] - User requested edit product 'Default'
[ERROR 2018-03-05 09:29] - The database for product 'Default_Dftul' cannot be connected to.
[ERROR 2018-03-05 09:29] - (sqlite3.OperationalError) unable to open database file
[ERROR 2018-03-05 09:29] - The configured connection for '/Default' failed: (sqlite3.OperationalError) unable to open database file
[INFO 2018-03-05 09:29] - Disconnecting product 'Default_Dftul'
It seems that a wrong product name is sent to the server?
@timeit | ||
def massStoreRun(self, name, tag, version, b64zip, force): | ||
self.__require_store() | ||
def __check_run_constraints(self, run_name): |
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.
Right now only the max run count is checked, could you rename the method to be more specific.
@gyorb I couldn't reproduce the above problem. I started a new web server with an empty workspace, updated the run limit but I didn't get any error. Could you please check your configuration database? Is it valid? |
6cfad13
to
4a8de8e
Compare
the server side error is not connected to these changes |
4a8de8e
to
7479554
Compare
Run limit can be configured on each product separately.
7479554
to
6c2fb85
Compare
Run limit can be configured on each product separately.