Skip to content

Fix issue #116#118

Merged
kbsingh merged 1 commit intoCentOS:masterfrom
mohammedzee1000:2017-01-18_12-14-12-rabbitmq_openshift
Jan 20, 2017
Merged

Fix issue #116#118
kbsingh merged 1 commit intoCentOS:masterfrom
mohammedzee1000:2017-01-18_12-14-12-rabbitmq_openshift

Conversation

@mohammedzee1000
Copy link
Copy Markdown
Contributor

@mohammedzee1000 mohammedzee1000 commented Jan 18, 2017

This is a fix for the issue #116

@mohammedzee1000 mohammedzee1000 force-pushed the 2017-01-18_12-14-12-rabbitmq_openshift branch from 5337da8 to e1e852a Compare January 18, 2017 16:55
@mohammedzee1000
Copy link
Copy Markdown
Contributor Author

mohammedzee1000 commented Jan 19, 2017

ping @jperrin

@mohammedzee1000 mohammedzee1000 force-pushed the 2017-01-18_12-14-12-rabbitmq_openshift branch from 6b10dd6 to 14e25b9 Compare January 19, 2017 11:27
@mohammedzee1000
Copy link
Copy Markdown
Contributor Author

mohammedzee1000 commented Jan 19, 2017

@surajssd fyi

scara
scara reviewed Jan 19, 2017
View reviewed changes
Comment thread rabbitmq/centos7/Dockerfile Outdated
@@ -43,7 +47,7 @@ ADD passwd.template /tmp/rabbitmq/passwd.template
RUN chown -R rabbitmq:0 /tmp/rabbitmq && chmod -R ug+rwx /tmp/rabbitmq
Copy link
Copy Markdown

@scara scara Jan 19, 2017

Choose a reason for hiding this comment

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

Should change rabbitmq into 1001.

Copy link
Copy Markdown
Contributor Author

@mohammedzee1000 mohammedzee1000 Jan 19, 2017

Choose a reason for hiding this comment

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

Thanks will update, hopefully wont affect function.

Copy link
Copy Markdown

@scara scara Jan 19, 2017

Choose a reason for hiding this comment

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

Yep 😉

@mohammedzee1000 mohammedzee1000 force-pushed the 2017-01-18_12-14-12-rabbitmq_openshift branch from ee89fc0 to 2a9bf23 Compare January 19, 2017 13:11
surajssd
surajssd reviewed Jan 20, 2017
View reviewed changes
Comment thread rabbitmq/centos7/Dockerfile Outdated
# Set permissions for openshift run
RUN chown -R 1001:0 /etc/rabbitmq && chown -R 1001:0 /var/lib/rabbitmq && chmod -R ug+rw /etc/rabbitmq && chmod -R ug+rw /var/lib/rabbitmq
RUN find /etc/rabbitmq -type d -exec chmod g+x {} +
RUN find /var/lib/rabbitmq -type d -exec chmod g+x {} +
Copy link
Copy Markdown

@surajssd surajssd Jan 20, 2017

Choose a reason for hiding this comment

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

can we reduce all those layers? I think most of these RUN statements can be clubbed into one or max two RUN statements?

Copy link
Copy Markdown
Contributor Author

@mohammedzee1000 mohammedzee1000 Jan 20, 2017

Choose a reason for hiding this comment

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

Will do updating

@mohammedzee1000 mohammedzee1000 force-pushed the 2017-01-18_12-14-12-rabbitmq_openshift branch from 2a9bf23 to bdff48a Compare January 20, 2017 06:13
@mohammedzee1000
Copy link
Copy Markdown
Contributor Author

mohammedzee1000 commented Jan 20, 2017

@surajssd updated

@mohammedzee1000
Copy link
Copy Markdown
Contributor Author

mohammedzee1000 commented Jan 20, 2017

ping @jperrin @kbsingh i believe this is ready

@concaf
Copy link
Copy Markdown

concaf commented Jan 20, 2017

@mohammedzee1000 tried this out, works for me, LGTM!

@mohammedzee1000
Copy link
Copy Markdown
Contributor Author

mohammedzee1000 commented Jan 20, 2017 •
edited
Loading

@containscafeine @surajssd @kbsingh fyi the issue itself has logs at the end showing the container is functional.

@concaf
Copy link
Copy Markdown

concaf commented Jan 20, 2017

@mohammedzee1000 nit: maybe squash the commits :)

@mohammedzee1000
Copy link
Copy Markdown
Contributor Author

mohammedzee1000 commented Jan 20, 2017 •
edited
Loading

@containscafeine Well there are no useless commits and ideally you dont want to squash unless there are intermediate commits directly related to something above them. Should i still squash?

@concaf
Copy link
Copy Markdown

concaf commented Jan 20, 2017

@mohammedzee1000 was just pointing out to keep the commit history better, but works either way. Just a nit.

@scara
Copy link
Copy Markdown

scara commented Jan 20, 2017

@mohammedzee1000 Usually commits should be atomic compared to the functionality they are supposed to fix/improve: here, the issue is that the container cannot work within Openshift and any of your commits properly addresses it showing your progresses w/ your own work and the advices/thoughts of the reviewers.
If I could vote, my +1 to squash them into 1 single commit reviewing the comments to be included (kind of intelligent fixup).

BTW Keep up the good work 👍 😄.

HTH,
Matteo

@surajssd
Copy link
Copy Markdown

surajssd commented Jan 20, 2017

@mohammedzee1000 yes LGTM :)

@mohammedzee1000
Copy link
Copy Markdown
Contributor Author

mohammedzee1000 commented Jan 20, 2017

Ok squashing

@mohammedzee1000
Fixed issue with rabbitmq container not working on openshift
25c3bfb 
The run failed due to random uid runs on openshift.
Added basic changes required to use nss_wrapper
Fixed issue with erlang cookie creation by exporting home.
Added volume mount and altered setup accordingly.
Restructured install to be based on openshift image build practices.
Updated readme with important information for debugging execs.
Changed username based permission setting to user id based.
Squashed multiple permission layers into 2.
Cleaned up dockerfile and added more comments.
@mohammedzee1000 mohammedzee1000 force-pushed the 2017-01-18_12-14-12-rabbitmq_openshift branch from d95bf00 to 25c3bfb Compare January 20, 2017 13:50
@mohammedzee1000
Copy link
Copy Markdown
Contributor Author

mohammedzee1000 commented Jan 20, 2017

Commits squashed @kbsingh good to merge

@kbsingh kbsingh merged commit 0ea2097 into CentOS:master Jan 20, 2017
@concaf concaf mentioned this pull request Jan 21, 2017
Open
concaf
concaf reviewed Jan 21, 2017
View reviewed changes
Comment thread rabbitmq/centos7/Dockerfile
# Send the logs to stdout
ENV RABBITMQ_LOGS=- RABBITMQ_SASL_LOGS=-

VOLUME /var/lib/rabbitmq
Copy link
Copy Markdown

@concaf concaf Jan 21, 2017

Choose a reason for hiding this comment

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

@mohammedzee1000 please refer to #116 (comment), this does not fix the issue for me :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@scara scara scara left review comments

@surajssd surajssd surajssd left review comments

@concaf concaf concaf left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mohammedzee1000 @concaf @scara @surajssd @kbsingh