[EmbeddedAnsible] Better handle ConfigurationScriptSource status/last_updated_on/last_update_error #19061
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@miq-bot add_label changelog/yes, core, embedded ansible, enhancement, ivanchuk/yes @Fryguy please review, since I think you will have the most to say with this one Note: This is also addressing this 'bug'/oversight that was brought up by @sbulage here: https://gitter.im/ManageIQ/manageiq-providers-embedded_ansible?at=5d354941437a3a134832eb4c cc @carbonin |
I would say yes. |
Fryguy
approved these changes
Jul 26, 2019
When a `EmbeddedAnsible::AutomationManager::ConfigurationScriptSource` is created and has been sync'd, have it update the `.status` field to be "successful", matching the `awx` results.
To simulate ansible/awx, set `.status` for ConfigurationScriptSource to 'new' when a new EmbeddedAnsible repo is first created. To do this, `CrudCommon.create_in_provider` was overwritten to append the `sync` action to the `super` call (instead of in `raw_create_in_provider`. Since these are two atomic actions, and a `Rugged::NetworkError` shouldn't cause the whole process to fail, splitting the code up like this allows a creation to happen, but it to fail on the sync without the record being lost and deleted in the process. As a result, there are a minor amount of extra DB calls in a successful use case, but it means that a user isn't required to resubmit the form fields to try again and can just do a provider refresh when there is a failure to re-attempt a sync.
Provides "error" status when a `ConfigurationScriptSource#sync` fails.
NickLaMuro
force-pushed
the
ansible_runner_fix_status_column
branch
from
2875ce9
to
e7ad3a5
Compare
NickLaMuro
changed the title
NickLaMuro
changed the title
|
Looks good to me, but could you fix the |
|
Sure, though the |
Unlike the ansible_tower provider, we haven't been implementing the `last_update_error` and `last_updated_on` values when syncing. This does the following: - Always updates `#last_updated_on` when completing a `#sync` - On error: Formats and sets `#last_update_error` - On success: Removes any previous errors from `#last_update_error` The ERROR_MAX_SIZE value is pulled from the `AnsibleTower` provider.
Splits up the actions for updating the database and refreshing the git repository into two atomic actions, similar to repo#create_in_provider. Also, to slightly DRY things up a bit, #sync_and_notify was added as well and was used in both the create and update methods.
NickLaMuro
force-pushed
the
ansible_runner_fix_status_column
branch
from
1acb803
to
1738d60
Compare
|
Checked commits NickLaMuro/manageiq@9c86b54~...1738d60 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0 app/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb
|
carbonin
approved these changes
Jul 29, 2019
simaishi
pushed a commit
that referenced
this pull request
Jul 30, 2019
…olumn [EmbeddedAnsible] Better handle ConfigurationScriptSource status/last_updated_on/last_update_error (cherry picked from commit 4e0783d)
|
Ivanchuk backport details: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, we don't set the status when doing a repo (ConfigurationScriptSource) sync. With this change, we now:
'new'status'running'at the beginning of#synclast_update_onis updated toTime.now'error'andlast_update_errorto provide the ruby error and stacktrace'successful'and clearlast_update_errorWorth noting: A "create" action is now done in two transactions and
.notify(fromCrudCommon). One for creating the record in the DB record and one for starting the sync. This is mostly because they really are two atomic actions. If a.create!passes, but the.syncfails, we shouldn't delete the record and just provide a notification in the UI and update the status.Also, if there was an error, a simple refresh of the provider will re-attempt the sync, or if there is something like a credential error, the user can update and the
.syncwill be fired again.TODO:
Handle translations (seperate PR?)