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 repository urls #570

Closed
wants to merge 2 commits into from
Closed

Update repository urls #570

wants to merge 2 commits into from

Conversation

aditsachde
Copy link
Contributor

@aditsachde aditsachde commented Mar 31, 2020

This repository used to be located at google/metallb and danderson/metallb. Redirects still work, but this just brings it in line with the current repository location.


Fixes #567

This repository used to be located at google/metallb and danderson/metallb. Redirects still work, but this just brings it in line with the current repository location.
@aditsachde
Copy link
Contributor Author

Didn't realize there was an issue for this already and a couple more locations to update the urls. Will fix and reopen.

@aditsachde aditsachde closed this Mar 31, 2020
@aditsachde aditsachde reopened this Mar 31, 2020
@rata
Copy link
Contributor

rata commented Apr 1, 2020

Hi! Thanks for the PR.

Can I ask what did you run to generate it? I'm mostly interested in that, as if very difficult to review otherwise (I don't know if there might be missing references by just looking at the diff).

Also, this has chances to break links in the website and manifests generation, so I'd like to be careful (maybe we can spit the changes?) and try to verify as much as possible? Have you done any kind of verification that things are sane after this PR?

@aditsachde
Copy link
Contributor Author

I did a find and replace of google/metallb and danderson/metallb to metallb/metallb and then went back and made sure all the changes were to Github urls (github.com or raw.githubusercontent.com). There are only a couple of exceptions.

In the README.md, there are three shields that depend on the repository url: license, Circle CI, and Go report card. Those still work with the updated urls.

Last place is here:

metallb/tasks.py

Lines 308 to 310 in 37586c4

_replace("/google/metallb/{}")
_replace("/google/metallb/tree/{}")
_replace("/google/metallb/blob/{}")

_replace seems to be a function that updates the references in website from the old version to the new version. Originally, it matched for google/metallb, but since those references have also been updated to metallb/metallb in the website files, it should still work. However, I am not sure about how to test this to make sure.

Because it tries to find those references in the website files, I don't think it is possible to update just the website urls without changing the patterns at the same time as then the script will not be able to find the correct references to update. (Maybe a better solution would be to add a second block and keep both google/metallb and metallb/metallb present in tasks.py?)

@rata
Copy link
Contributor

rata commented Apr 28, 2020

Thanks a lot! But do you have any script (even if afterward you did manual changes) to achieve this diff?

then went back and made sure all the changes were to Github urls (github.com or raw.githubusercontent.com). There are only a couple of exceptions.

Exceptions like places were we didn't want to change it? Do you happen to have them handy?

_replace seems to be a function that updates the references in website from the old version to the new version. Originally, it matched for google/metallb, but since those references have also been updated to metallb/metallb in the website files, it should still work. However, I am not sure about how to test this to make sure.

That is used when creating a release. We can do it locally, no need to actually create a release. But maybe we can keep them and see that they actually didn't change anything and also add the new ones using metallb github org (as the code should have been updated in this PR).

Because it tries to find those references in the website files, I don't think it is possible to update just the website urls without changing the patterns at the same time as then the script will not be able to find the correct references to update. (Maybe a better solution would be to add a second block and keep both google/metallb and metallb/metallb present in tasks.py?)

Yeah, I think we should keep them for a few releases, just to be safe :)

These changes are not top prio now, though, and is important to not break the links. So, I'd be slower to review this. Let me know if this is a problem in any way for you.

Not sure if it's worth doing some scripts to verify (like running locally with hugo and follow links with a "bot", or something alike. Will sleep on this :)

Thanks again!

@aditsachde
Copy link
Contributor Author

I just used the vscode find and replace, so no script.

The only things that weren't github urls (i.e. the exceptions) were the readme shields, and those were still updated to point to the new url. Using some scripts to check if everything works would be cool, but I don't know if that would really be faster than just having another set of eyes do a sanity check.

I don't really know how to test the release stuff, so ideally someone more experienced with the codebase would check it but I know that everyone is quite busy at the moment.

As you mentioned, this is quite a low priority task as all the redirects are still working and there are much more important things to spend time on right now.

@caboteria
Copy link
Contributor

LGTM. I can vouch for the fact that this is a "papercut" that's confusing to new users. Russellb already merged a PR that implemented part of this (the widgets on the readme page) but this PR would be helpful to newbies.

@caboteria
Copy link
Contributor

LGTM cc @rata

@rata
Copy link
Contributor

rata commented Jul 22, 2020

@caboteria as I mentioned here and in the community call today. I'd like to verify that this doesn't break the links in the project prior to merge. And as there are several other things have more priority (as important bug fixes), I didn't look at this yet.

If you want to check and share why this doesn't break any links in a way than others can review, that would be great. If you don't want to do it, is okay too. I'll do it when other very important bugs are fixed :)

Copy link
Contributor

@daxmc99 daxmc99 left a comment

Choose a reason for hiding this comment

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

This looks like a straightforward find and replace. I think we should go ahead and merge this. I tested a few of the new links and they all work 😄
Thanks!

- BGP peers now have
a
[node selector]({{% relref "configuration/_index.md" %}}#limiting-peers-to-certain-nodes). You
can use this to integrate MetalLB into more complex cluster network
topologies.
- MetalLB now has
a
[Helm chart](https://github.com/google/metallb/tree/main/helm/metallb). If
[Helm chart](https://github.com/metallb/metallb/tree/main/helm/metallb). If
Copy link
Member

@johananl johananl Aug 4, 2020

Choose a reason for hiding this comment

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

This gives a 404 (but the old link does, too). I'm not sure we want to fix this - it's in the notes of an old release.

Copy link
Member

@johananl johananl left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your contribution @aditsachde.

I've tested all the links and they seem to work. The only thing I couldn't test is the _replace function in tasks.py:

https://github.com/metallb/metallb/pull/570/files#diff-dc31105b816206de63c47bf42ec2a4d7R308-R310

If we can test this without crafting a release, let's do it and then merge. If we can't, let's merge as is and fix any problems which arise when we encounter them.

@johananl
Copy link
Member

johananl commented Aug 4, 2020

Tried to rebase this but looks like the fork has been deleted. I'll open a new PR with the patch.

@johananl johananl mentioned this pull request Aug 5, 2020
@johananl
Copy link
Member

johananl commented Aug 5, 2020

Tried to rebase this but looks like the fork has been deleted. I'll open a new PR with the patch.

Closing in favor of #688.

@johananl johananl closed this Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace danderson/metallb with metallb/metallb
6 participants