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

Use constant for loopback address #2629

Merged
merged 2 commits into from Apr 8, 2021

Conversation

piotr-roslaniec
Copy link
Member

@piotr-roslaniec piotr-roslaniec commented Apr 8, 2021

Type of PR:

  • Bugfix
  • Feature
  • Documentation
  • Other - (Code Quality)

Required reviews:

  • 1
  • 2
  • 3

What this does:

  • Replaces occurrences of 127.0.0.1 with a LOOPBACK_ADDRESS constant.

Issues fixed/closed:

Why it's needed:

Explain how this PR fits in the greater context of the NuCypher Network.
E.g., if this PR address a nucypher/productdev issue, let reviewers know!

Notes for reviewers:

  • One occurrence of 127.0.0.1 was ignored because it's a script without dependency on the codebase.

@piotr-roslaniec piotr-roslaniec changed the title [WIP] Use constant for loopback address Use constant for loopback address Apr 8, 2021
@piotr-roslaniec piotr-roslaniec marked this pull request as ready for review April 8, 2021 12:27
@derekpierre
Copy link
Member

Great issue to tackle - @piotr-roslaniec you're on fire 🔥 !

There have been some discussions (and disagreements) around which PRs require a newsfragment. I think changes that are internal / code quality / testing / refactoring changes don't need one unless they directly affect users (app developers or node operators).

In this case, I don't think we need one, but others can speak up if they disagree. Definitely something we need to reach agreement on and officially document - perhaps within the README https://github.com/nucypher/nucypher/blob/main/newsfragments/README.md.

@piotr-roslaniec
Copy link
Member Author

Thanks, @derekpierre!
I was under an impression that newsfragments are a hard requirement for any contribution (due to GH Action checks). But I think I agree with your judgment.
I actually intended to tackle a related issue: #2571. Maybe we could have a discussion in a wider circle there?

Copy link
Member

@KPrasch KPrasch left a comment

Choose a reason for hiding this comment

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

🤠

@KPrasch KPrasch added the Code Quality 🔧 Pertaining to code quality improvements label Apr 8, 2021
Copy link
Member

@derekpierre derekpierre 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

GH action check is there just to ensure that the addition/non-addition of a newsfragment has been thought about - it is tough to have it to understand PR context 😕

@@ -0,0 +1 @@
Use constant for loopback address across the codebase.
Copy link
Member

Choose a reason for hiding this comment

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

Have a look at the towncrier documentation (https://github.com/twisted/towncrier#philosophy). IMO this PR does not need a newsfragment. If we had a formal changelog I think it might be better suited there. In any case, I prefer to err on the side of verbosity.

@KPrasch KPrasch merged commit 05ab5d3 into nucypher:main Apr 8, 2021
@piotr-roslaniec piotr-roslaniec deleted the loopback-addr#2538 branch April 9, 2021 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Quality 🔧 Pertaining to code quality improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use constant for loopback address (127.0.0.1) across codebase
3 participants