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

Update the config docs and add info on configuration for public access #149

Merged
merged 5 commits into from Apr 2, 2019

Conversation

jeremycline
Copy link
Member

This has a couple small commits which I didn't feel was worth the effort of breaking out into numerous PRs, but all center around making the configuration documentation clearer, including how to configure access to the Fedora broker.

@codecov-io
Copy link

codecov-io commented Mar 26, 2019

Codecov Report

Merging #149 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #149   +/-   ##
=======================================
  Coverage   95.95%   95.95%           
=======================================
  Files          14       14           
  Lines        1408     1408           
  Branches      189      189           
=======================================
  Hits         1351     1351           
  Misses         44       44           
  Partials       13       13
Impacted Files Coverage Δ
fedora_messaging/config.py 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c69761...8174f84. Read the comment docs.

Copy link
Member

@abompard abompard left a comment

Choose a reason for hiding this comment

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

It looks good, thanks, just a couple questions.

ca_cert = "/etc/pki/tls/certs/ca-bundle.crt"
keyfile = "/my/client/key.pem"
certfile = "/my/client/cert.pem"
ca_cert = "/etc/fedora-messaging/fedora.cacert.pem"
Copy link
Member

Choose a reason for hiding this comment

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

In the other example config files, this file is called cacert.pem. Is it intended to have a different name here?

certfile = "/my/client/cert.pem"
ca_cert = "/etc/fedora-messaging/fedora.cacert.pem"
keyfile = "/etc/fedora-messaging/fedora.key.pem"
certfile = "/etc/fedora-messaging/fedora.cert.pem"
Copy link
Member

Choose a reason for hiding this comment

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

In the other example config files, those files use a dash between fedora and the file type. Is it intended to have a different name here?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I just adjusted the name repeatedly and they got out of sync. Fixed.

#
# This file is in the TOML format.
amqp_url = "amqps://fedora:@rabbitmq.fedoraproject.org/%2Fpublic_pubsub"
passive_declares = false
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't that be true?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it needs to be false because in the public_pubsub vhost users create their own queues. Since the default is false I think this should be dropped here, though. Looked like I dropped it from the stg config, but not here.

Copy link
Member

Choose a reason for hiding this comment

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

Right, I mixed it up. Do you think there may be errors trying to declare the exchange?

Copy link
Member Author

Choose a reason for hiding this comment

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

It works fine in staging. I'm having an issue in production that looks like a proxy problem - the initial TCP handshake isn't completing, but it's the same configuration.

# This file is in the TOML format.
amqp_url = "amqps://fedora.stg:@rabbitmq.stg.fedoraproject.org/%2Fpublic_pubsub"
callback = "fedora_messaging.example:printer"

Copy link
Member

Choose a reason for hiding this comment

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

Should we set passive_declares to true here?

This shuffles the example configuration file around so that each section
of configuration in the example matches up with the documentation in
terms of order. It also inlines the example into the Sphinx
documentation.

Signed-off-by: Jeremy Cline <jcline@redhat.com>
Signed-off-by: Jeremy Cline <jcline@redhat.com>
Signed-off-by: Jeremy Cline <jcline@redhat.com>
Signed-off-by: Jeremy Cline <jcline@redhat.com>
Add credentials and configurations for both the staging and production
virtual hosts provided by Fedora. Document their usage.

Signed-off-by: Jeremy Cline <jcline@redhat.com>
Copy link
Member

@abompard abompard left a comment

Choose a reason for hiding this comment

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

Looks good, thanks a lot and sorry for the delay!

@mergify mergify bot merged commit 8a8b2d6 into fedora-infra:master Apr 2, 2019
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

3 participants