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

Move weaveworks/common back to upstream #945

Closed
cyriltovena opened this issue Aug 28, 2019 · 9 comments · Fixed by #1226
Closed

Move weaveworks/common back to upstream #945

cyriltovena opened this issue Aug 28, 2019 · 9 comments · Fixed by #1226
Labels
good first issue These are great first issues. If you are looking for a place to start, start here! help wanted We would love help on these issues. Please come help us! lifecycle/upstream A change must be done to an upstream project first

Comments

@cyriltovena
Copy link
Contributor

To not block some PR we moved weaveworks/common to a personal fork. Once weaveworks/common#153 is merged we can go back to upstream.

@cyriltovena cyriltovena added help wanted We would love help on these issues. Please come help us! good first issue These are great first issues. If you are looking for a place to start, start here! labels Aug 28, 2019
@sh0rez sh0rez added the lifecycle/upstream A change must be done to an upstream project first label Sep 10, 2019
@ThoreKr
Copy link

ThoreKr commented Sep 27, 2019

As weaveworks/common#153 is more or less dead and waiting for a rebase I reopened a new (rebased) PR (weaveworks/common#167).

@clickyotomy
Copy link
Contributor

Hello! Can I work on this after weaveworks/common#167 is merged?

@ThoreKr
Copy link

ThoreKr commented Oct 1, 2019

@clickyotomy If you are more familiar with the dependencies a bit of help in weaveworks/common#167 would be most welcome.

Initially it was only meant to be a rebase to resolve a merge conflict, but now the actual change has been reviewed again.

@ThoreKr
Copy link

ThoreKr commented Oct 1, 2019

@clickyotomy Please also take a look at my notes in #1051 as there is apparently another version in the vendor subdirectory.

@clickyotomy
Copy link
Contributor

@ThoreKr, I am not familiar with the changes introduced in weaveworks/common#167, but I will try to help. :)

  • I don't know what needs to be done here - should we include/replace localhost on all the tests?
  • However, here - maybe we can get rid of HTTPListen{Port,Address} for TestErrorInstrumentationMiddleware, because it's probably not needed for that test.

How do I make changes to the PR? Fork from https://github.com/ThoreKr/weaveworks-common/tree/server-listen-addr and open a new one?

Regarding #1051, this repo was moved to go modules (#1062) for dependency management. We might just have to do a go mod edit -dropreplace=personal_fork[@v] and use the latest from upstream for this issue.

@ThoreKr
Copy link

ThoreKr commented Oct 1, 2019

@clickyotomy Me neither, I just rebased it. Maybe wait for comments from upstream whether it should be tested or not.

@stale
Copy link

stale bot commented Oct 31, 2019

This issue has been automatically marked as stale because it has not had any activity in the past 30 days. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale A stale issue or PR that will automatically be closed. label Oct 31, 2019
@ThoreKr
Copy link

ThoreKr commented Nov 1, 2019

Still waiting for weaveworks-common.

Starting to get annoyed.

@stale stale bot removed the stale A stale issue or PR that will automatically be closed. label Nov 1, 2019
@ThoreKr
Copy link

ThoreKr commented Nov 3, 2019

@clickyotomy it's merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue These are great first issues. If you are looking for a place to start, start here! help wanted We would love help on these issues. Please come help us! lifecycle/upstream A change must be done to an upstream project first
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants