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

Projects
None yet
3 participants
@jeremycline
Copy link
Member

commented Mar 26, 2019

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

This comment has been minimized.

Copy link

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.

@abompard
Copy link
Member

left a comment

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"

This comment has been minimized.

Copy link
@abompard

abompard Mar 27, 2019

Member

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"

This comment has been minimized.

Copy link
@abompard

abompard Mar 27, 2019

Member

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?

This comment has been minimized.

Copy link
@jeremycline

jeremycline Mar 27, 2019

Author Member

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

This comment has been minimized.

Copy link
@abompard

abompard Mar 27, 2019

Member

Shouldn't that be true?

This comment has been minimized.

Copy link
@jeremycline

jeremycline Mar 27, 2019

Author Member

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.

This comment has been minimized.

Copy link
@abompard

abompard Mar 28, 2019

Member

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

This comment has been minimized.

Copy link
@jeremycline

jeremycline Mar 28, 2019

Author Member

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"

This comment has been minimized.

Copy link
@abompard

abompard Mar 27, 2019

Member

Should we set passive_declares to true here?

@jeremycline jeremycline force-pushed the jeremycline:config-docs branch from e215792 to c38f0ff Mar 27, 2019

jeremycline added some commits Mar 25, 2019

Re-order the config documentation and example config
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>
docs/installation: Linkify the PyPI reference
Signed-off-by: Jeremy Cline <jcline@redhat.com>
Note that Twisted ignores the connection-related URL params
Signed-off-by: Jeremy Cline <jcline@redhat.com>
Add an example of using the consumer configuration key
Signed-off-by: Jeremy Cline <jcline@redhat.com>
Add the credentials and configs for the Fedora public vhosts
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>

@jeremycline jeremycline force-pushed the jeremycline:config-docs branch from c38f0ff to 8174f84 Apr 2, 2019

@abompard
Copy link
Member

left a comment

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

@mergify mergify bot merged commit 8a8b2d6 into fedora-infra:master Apr 2, 2019

3 checks passed

DCO DCO
Details
Mergify — Summary 2 rules match
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.