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

Added the missing package for abrt #109

Merged
merged 1 commit into from
Jan 12, 2015
Merged

Conversation

tkolhar
Copy link
Contributor

@tkolhar tkolhar commented Jan 12, 2015

This step is missing :
https://github.com/theforeman/foreman_abrt/blob/katello-docs/README.md#installing-the-foreman-plugin

Added as per the instructions given in README
please review it thanks

@sghai
Copy link
Contributor

sghai commented Jan 12, 2015

ACK

@kbidarkar
Copy link
Collaborator

ACK. Will leave it as it is for elyezer to merge.

]
for package in packages:
run('yum install -y {0}'.format(package))

run('systemctl restart foreman')
run('touch /usr/share/foreman/tmp/restart.txt')
Copy link
Contributor

Choose a reason for hiding this comment

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

The above will restart foreman, is there anything else touching the restart file does?

By having both foreman will be restarted twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elyezer it only restarts the foreman as per the discussion with the developer

Copy link
Contributor

Choose a reason for hiding this comment

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

touch is either to create empty file or to change the timestamps of file..forman is already restarting at line 402

Copy link
Contributor

Choose a reason for hiding this comment

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

@tkolhar then I think is better to remove this line as @sghai said, foreman is already restarting at line 402.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sghai @elyezer : I had a talk with the developer
this was work around provided to me in case foreman service does
not gets restarted with systemctl

@elyezer
Copy link
Contributor

elyezer commented Jan 12, 2015

ACK, pending comment.

elyezer added a commit that referenced this pull request Jan 12, 2015
Added the missing package for abrt
@elyezer elyezer merged commit d43b23c into SatelliteQE:master Jan 12, 2015
@omaciel
Copy link
Member

omaciel commented Jan 12, 2015

@tkolhar @elyezer @sghai @kbidarkar one minor but important issue here is that, if we add a workaround to our code, we have to make sure that:

  • There a BZ associated with the issue
  • We don't use the workaround during the release cycle

@elyezer
Copy link
Contributor

elyezer commented Jan 12, 2015

@omaciel good point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants