Skip to content
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

Add configuration_script_sources.last_update_error #187

Conversation

Glutexo
Copy link
Member

@Glutexo Glutexo commented Apr 12, 2018

Add new last_update_stdout column to configuration_script_sources table. This allows to store last update stdout to display more information about why its refresh failed.

Fetching and presenting this information is a workaround for cloning repositories from HTTPS servers with untrusted SSL certificates. Tower API does not offer an option to disable SSL check, so with the stdout user can at least see, why the cloning actually failed.

https://bugzilla.redhat.com/show_bug.cgi?id=1513616

Followed by adding the column to model: #17290 in manageiq.

/cc @jameswnl for review

@lpichler
Copy link
Contributor

@Glutexo I see that there is lot of related PRs to the BZ. Would you like to create GH issue for tracking it ? It is not clear what need to be merged first.
I prefer checkbox list(each checkbox related to one PR) in the GH issue.

@Glutexo
Copy link
Member Author

Glutexo commented May 4, 2018

@miq-bot add_reviewer @jameswnl

Can this get merged, please? There are no dependencies for this and it has been hanging there for a long time. Who might have write access to this repository? Maybe @lpichler or @Ladas?

@miq-bot miq-bot requested a review from jameswnl May 4, 2018 10:46
@Ladas
Copy link
Contributor

Ladas commented May 4, 2018

@Glutexo how big can the output be? @Fryguy is the one usually merging migrations. Other option here would be to use BinaryBlob class.

@Glutexo
Copy link
Member Author

Glutexo commented May 4, 2018

@Ladas Typical Ansible outputs are a few (say, 1–15) kilobytes. Since TEXT columns are (almost) unlimited and the stdout is text (not binary), I’d stick with this type. But maybe there are some implications I’m not aware of?

Copy link
Contributor

@jameswnl jameswnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am ok with this. Need @Fryguy's review

@jameswnl
Copy link
Contributor

@miq-bot add_reviewer @Fryguy

@miq-bot miq-bot requested a review from Fryguy May 10, 2018 14:06
@Glutexo
Copy link
Member Author

Glutexo commented May 22, 2018

Just pinging @Fryguy. Is this PR ok to be merged? There are other PRs that depend on this one. See ManageIQ/manageiq#17307

@Glutexo
Copy link
Member Author

Glutexo commented Jun 7, 2018

@Fryguy Hey, may I ask you for a review/merge of this PR? It’s required by ManageIQ/manageiq#17290 and through that by other PRs too. Thanks!

@@ -0,0 +1,5 @@
class AddLastUpdateStdoutToConfigurationScriptSource < ActiveRecord::Migration[5.0]
def change
add_column :configuration_script_sources, :last_update_stdout, :text
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bdunne Aside from my naming questions in ManageIQ/manageiq#17290 (comment), does this even make sense on configuration_script_sources? I thought sources was the content of the configuration script. But content is not invocation, and I'd expect an "output" field on the "runtime invocation" model.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See ManageIQ/manageiq#17290 (comment) maybe we should continue the conversation over there?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still store some data concerning the last update: status and timestamp. The only difference is that in Tower these are propagated to the project itself, so they are accessible even without explicitly asking for the last update resource.

@Glutexo Glutexo force-pushed the add_configuration_script_source_last_update_stdout branch from 32059cc to f085d3a Compare June 20, 2018 12:55
Add new last_update_error column to configuration_script_sources table.
This allows to store last update stdout to display more information
about why its refresh failed.
@Glutexo Glutexo force-pushed the add_configuration_script_source_last_update_stdout branch from f085d3a to 7a5c1de Compare June 21, 2018 07:14
@Glutexo
Copy link
Member Author

Glutexo commented Jun 21, 2018

@Fryguy Renamed to last_update_error. Ready to merge?

@miq-bot
Copy link
Member

miq-bot commented Jun 21, 2018

Checked commit Glutexo@7a5c1de with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

@Fryguy Fryguy changed the title Add configuration_script_sources.last_update_stdout Add configuration_script_sources.last_update_error Jun 27, 2018
@Fryguy Fryguy merged commit 3d5c661 into ManageIQ:master Jun 27, 2018
@Fryguy Fryguy added this to the Sprint 89 Ending Jul 2, 2018 milestone Jun 27, 2018
@Fryguy Fryguy self-assigned this Jun 27, 2018
@Glutexo Glutexo deleted the add_configuration_script_source_last_update_stdout branch June 28, 2018 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants