1452694: Candlepin no longer deletes pools for custom subscriptions (2.0) #1632
Conversation
Ceiu
changed the title
Ceiu
force-pushed
the
crog/pool_migration_fix-20
branch
2 times, most recently
from
906f621
to
1e6610c
Compare
server/spec/import_spec.rb
Outdated
| @@ -120,7 +120,8 @@ def import_and_wait | |||
| it 'can be undone' do | |||
| # Make a custom pool so we can be sure it does not get wiped | |||
| # out during either the undo or a subsequent re-import: | |||
| prod = create_product(random_string(), random_string(), {:owner => @import_owner['key']}) | |||
| prod_name = "custom_pool_prod-" + random_string() | |||
There was a problem hiding this comment.
Just for future reference, random_string("custom_pool_prod-") would accomplish the same thing I believe. Don't worry about changing it but I think the more concise usage would be preferable in the future.
There was a problem hiding this comment.
Gotcha. I'll address this here since there are other changes to make anyway.
|
|
||
| // TODO: | ||
| // Remove the standalone config check and replace it with a check for whether or not the | ||
| // pool is "managed" -- however we decide to implement that in the future. |
There was a problem hiding this comment.
Could we go ahead and implement isManaged() and just have it return !standalone || pool.getUpstreamPoolId() != null? That way there's one obvious place to go when we decide what being managed actually is.
There was a problem hiding this comment.
Do we have a convenient way to have a model object fetch the config without requiring it be passed in explicitly? As it is now, that check is part of the "is managed" bit.
There was a problem hiding this comment.
I think you could get it out of the resteasy context. I'm pretty sure we put it in there
but not sure if we really want to pollute the model with looking into the app's config.
Getting it from the context would be like public boolean isManaged(@Context Config config) if I recall correctly.
There was a problem hiding this comment.
I'm not really sure that I like the config leaking into the model object. Could we just set 'managed' appropriately on the pool when it is created via the API vs a refresh (not now, but later on)? Then it would be easy to determine at a database level.
There was a problem hiding this comment.
While such a change would be optimal, I feel it would be a rather large change, and one I'm not sure we want to do with this PR. It's something that's better done with the pool hierarchy task.
There was a problem hiding this comment.
@Ceiu Agreed, I didn't intend it to be done now.
There was a problem hiding this comment.
I wonder if we can split the difference and add the isManaged method to CandlepinPoolManager (which UndoImportsJob uses) with a TODO to move it to Pool when the logic is no longer reliant upon the config for proper operation.
There was a problem hiding this comment.
@Ceiu something like poolManager.isManaged(pool)? I'd be fine with that.
| @@ -112,7 +112,7 @@ public void compileRules(boolean forceRefresh) { | |||
| return; | |||
| } | |||
|
|
|||
| log.info("Recompiling rules with timestamp: " + newUpdated); | |||
| log.info("Recompiling rules with timestamp: {}", newUpdated); | |||
- Pools for custom subscriptions will no longer be deleted on manifest delete/undo import - The Per-org products migration has been updated/fixed to also migrate subscription information from the subscription table to the pool table - Bonus pools now inherit upstream pool and customer data from their source pool
Ceiu
force-pushed
the
crog/pool_migration_fix-20
branch
from
1e6610c
to
13b8660
Compare
delete/undo import
subscription information from the subscription table to the pool table
source pool