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

Fixes #455 - Added documentation for modifying message replay mechanism #492

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

cha-ku
Copy link
Contributor

@cha-ku cha-ku commented Nov 15, 2017

Added the documentation for changing replay mechanism in subscribing.rst

@codecov
Copy link

codecov bot commented Nov 15, 2017

Codecov Report

Merging #492 into develop will increase coverage by 0.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop     #492     +/-   ##
==========================================
+ Coverage    60.06%   60.17%   +0.1%     
==========================================
  Files           30       29      -1     
  Lines         1753     1750      -3     
  Branches       272      272             
==========================================
  Hits          1053     1053             
+ Misses         623      620      -3     
  Partials        77       77

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 78c0ffa...014b020. Read the comment docs.

Copy link
Member

@jeremycline jeremycline left a comment

Choose a reason for hiding this comment

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

Two small issues, but it's looking good.

Please don't close the pull request as it loses the review history when you do that. Just push a new commit or amend your current commit and force push (preferred).

Thanks!

messages, and store them all in the db.

::
messages, and store them all in the db.::
Copy link
Member

Choose a reason for hiding this comment

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

The changes in this file aren't related to the rest of the commit and don't seem necessary to me. Please don't include them in this commit. Thanks!

missed from the datagrepper service. To disable this functionality:

#. Stop fedmsg-hub
#. Remove the file at on disk at ``/var/run/fedmsg/status/fedmsg-hub/*``
Copy link
Member

Choose a reason for hiding this comment

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

This should be "Remove the file on disk at" rather than "Remove the file at on disk at"

@cha-ku
Copy link
Contributor Author

cha-ku commented Dec 9, 2017

@jeremycline Thanks for the review! I have made the necessary changes. I think this should be fine now. Please let me know if there are any issues. Thanks again!

@pypingou
Copy link
Member

pypingou commented Dec 9, 2017

There seems to be a merge commit in there, maybe try using git rebase ?

@glensc
Copy link
Contributor

glensc commented Apr 3, 2018

the merge commit is there because developer didn't create feature branch, thus this PR is linked to his develop branch.

@glensc
Copy link
Contributor

glensc commented Apr 3, 2018

i think it's simpler for maintainers to cherry pick and fixup the commit and be done with it.

@cha-ku
Copy link
Contributor Author

cha-ku commented May 8, 2018

@jeremycline @glensc @pypingou Do I need to rebase my changes on top of current develop branch? Or would it be easier to open a new PR?

@pypingou
Copy link
Member

Rebasing and force-pushing to your fork should be straight forward, but the new PR road is always an option

jeremycline and others added 12 commits May 18, 2018 01:30
This reverts commit 2a6148a.

Since the CLI is not currently hooked up to anything, revert this commit
before releasing v1.1.0.

Signed-off-by: Jeremy Cline <[email protected]>
With the recent change to how CA certificates are loaded, just use the
path to certificate.

Signed-off-by: Jeremy Cline <[email protected]>
* fix: Use int validator for ca and crl cache expiry

* fix: Give expiry keys sane defaults

* test: Update expiry defaults

* fix: Update relay_inbound references to use str

* fix: Set crl_cache_expiry to a better default
In a non-zeromq case, it matters what the return value of _consume is.  (It is
a boolean, indicating if the message was successfully handled or not).  This,
in turn, gets used to figure out whether or not to send an ACK back to the
broker.

Here, the return values were being swallowed and turned into `None` values.
Signed-off-by: Jeremy Cline <[email protected]>
This will make python 3.6+ happier and stop sending DeprecationWarning

Signed-off-by: Pierre-Yves Chibon <[email protected]>
This moves the documentation for each configuration value into the
module docblock and alters the Sphinx auto-generated documentation to
include the module-level documentation in the user-facing configuration
page. The configuration API docs used to be the place where
configuration load order was documented and users weren't finding it.

The documentation itself is not changed.

Fixes #505

Signed-off-by: Jeremy Cline <[email protected]>
@cha-ku
Copy link
Contributor Author

cha-ku commented May 17, 2018

I did a git rebase and then pushed the fork on top. Hope this is alright.

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.

6 participants