-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[constructed-inventory] Allow filtering based on facts #13678
[constructed-inventory] Allow filtering based on facts #13678
Conversation
log_data['inventory_id'] = inventory.id | ||
log_data['written_ct'] = 0 | ||
try: | ||
os.makedirs(destination, mode=0o700) |
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.
nit: Can we use exist_ok=True
and skip the try/except? Was introduced in 3.2
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.
The reason the except is needed here but not before is because we loop over input_inventories
, so the dir will exist after the first one. I've thought that we could refactor this away instead.
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'll keep thinking about this but will go ahead with the merge for now.
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 I agree that using exist_ok=True
is a bit cleaner here. It effectively does the same thing, just avoids throwing the exception at all, so the try/except/pass lines become unnecessary.
hosts_to_update.append(host) | ||
system_tracking_logger.info('Facts cleared for inventory {} host {}'.format(smart_str(host.inventory.name), smart_str(host.name))) | ||
log_data['cleared_ct'] += 1 | ||
if len(hosts_to_update) > 100: |
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.
nit: being able to inject the pivot number could make unit/integration testing easier: e.g., set it to 1 or 2 for a test and don't have to have 100 hosts.
@@ -1557,6 +1579,9 @@ def build_args(self, inventory_update, private_data_dir, passwords): | |||
|
|||
return args | |||
|
|||
def should_use_fact_cache(self): | |||
return bool(self.instance.source == 'constructed') |
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.
Why the bool()
cast here?
log_data['inventory_id'] = inventory.id | ||
log_data['written_ct'] = 0 | ||
try: | ||
os.makedirs(destination, mode=0o700) |
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 I agree that using exist_ok=True
is a bit cleaner here. It effectively does the same thing, just avoids throwing the exception at all, so the try/except/pass lines become unnecessary.
* initial functional filter-on-facts functionality * Move facts to its own module to make interface more coherent * Update test
* initial functional filter-on-facts functionality * Move facts to its own module to make interface more coherent * Update test
* initial functional filter-on-facts functionality * Move facts to its own module to make interface more coherent * Update test
* initial functional filter-on-facts functionality * Move facts to its own module to make interface more coherent * Update test
* initial functional filter-on-facts functionality * Move facts to its own module to make interface more coherent * Update test
* initial functional filter-on-facts functionality * Move facts to its own module to make interface more coherent * Update test
SUMMARY
The main diff here is that, since we are using facts for inventory update prep, they no longer "belong" to jobs, so I moved it into its own module. It is still awkward to manage the interface at times, but this is the most sense I can make of it.
This is mostly written in iteration with an integration test for it, unit tests need work as of opening this.
ISSUE TYPE
COMPONENT NAME