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

Bulk change "http://..." to "https://..." #463

Closed
larsbarring opened this issue Oct 16, 2023 · 15 comments
Closed

Bulk change "http://..." to "https://..." #463

larsbarring opened this issue Oct 16, 2023 · 15 comments
Assignees
Labels
change agreed Issue accepted for inclusion in the next version and closed defect Conventions text meaning not as intended, misleading, unclear, has typos, format or language errors
Milestone

Comments

@larsbarring
Copy link
Contributor

Following a recent comment by @DocOtak I checked the files in this repo for the obsoleted http:/ vs. the current practice https://:

$ grep "http://" ./*.{adoc,md} | wc -l
64
$ grep "https://" ./*.{adoc,md} | wc -l
43

Would it be possible to do a bulk update of to bring all links up-to-date to use https:// ?

@sadielbartholomew
Copy link
Member

Thanks Lars. I'm glad someone is suggesting this. I noticed a while back that we were usually referring to links with HTTP which, as per Andrew's comment, is indeed quite antiquated. I would support getting this updated (though as an infrastructure/admin change I doubt it needs the approval).

I can't see any reason we can't make the update like you suggest, since any updated link would always point to the same place but go there with a more secure and standard protocol.

@larsbarring
Copy link
Contributor Author

As I am already outside my comfort zone I was hoping that someone from the CF Information Management and Support Team would pick up this :-)

@sadielbartholomew
Copy link
Member

Aha, well in that case I am happy to volunteer (I am in that team), though I've got quite a lot on at the moment so probably won't be able to do it until next week at the earliest. Does that work?

@larsbarring
Copy link
Contributor Author

Thanks Sadie! (For the record: Despite your quick response, I wasn't at all specifically aiming at you in my previous post.) And personally I think that getting #445 into v1.11 is more important.

@sadielbartholomew
Copy link
Member

sadielbartholomew commented Oct 18, 2023

For the record: Despite your quick response, I wasn't at all specifically aiming at you in my previous post

No worries, Lars, it was clear you weren't but regardless I was and I am happy to volunteer because the change is non-controversial and should be quick to make via sed replacement so no won't take long. And the reference to HTTP has long stuck out to me as wrong so it will be good to fix that up. I assign myself here to show that I'll sort it (not immediately but soon enough, by next week).

@sadielbartholomew sadielbartholomew self-assigned this Oct 18, 2023
@larsbarring larsbarring added the defect Conventions text meaning not as intended, misleading, unclear, has typos, format or language errors label Oct 18, 2023
@Dave-Allured
Copy link
Contributor

Dave-Allured commented Oct 18, 2023

I suggest that every changed link be checked for functionality, before committing merging. Occasionally I still run into HTTP functioning websites. Just my opinion, not a blocker.

@larsbarring
Copy link
Contributor Author

larsbarring commented Oct 19, 2023

Yes, I think this is a good idea.
And I always wondered why we do not have a link checker kicking in with some regularity (weekly, 15 days, monthly, ...) to check also the conventions document, much as is now done for the CF website. But that is a matter for another issue.

@davidhassell
Copy link
Contributor

Hello - just a friendly reminder that this would have to be implemented by 12th November 2023 to make it into CF-1.11. Thanks!

@sadielbartholomew
Copy link
Member

Thanks for the reminder, David. I will put up a PR by the end of the working week latest, in that case.

@sadielbartholomew
Copy link
Member

The PR is now up for this: see #476. Thanks.

@larsbarring
Copy link
Contributor Author

Excellent work Sadie!
I commented in the PR regarding the problematic links that you found.

@larsbarring
Copy link
Contributor Author

larsbarring commented Nov 15, 2023

After a few minor technical adjustments to the PR #476, I think these purely housekeeping changes are ready to be merged. Thanks @Dave-Allured for pointing at the possibility of http links without a https counterpart.

Because the PR in no way influences the conventions as such I do not think that we have to wait the three weeks before accepting it. What do you think @davidhassell and @JonathanGregory?

@JonathanGregory
Copy link
Contributor

Dear @sadielbartholomew and @larsbarring

Thanks for this very useful and careful piece of work.

Once we have completed this release, we ought to think a bit about the rules which should apply for accepting minor changes. However, my understanding is that "acceptance" refers to agreement about the principle and the wording of the change to be made. It does not refer to the PR. We don't have any rules about the timescales for PRs, because our rules were agreed before we moved to GitHub. The PR is "just" the implementation of an agreed change. No-one objected and enough people supported this change being made a month ago, when you first proposed it, so I think it already qualifies as "accepted" and it's fine to merge the PR now.

Cheers

Jonathan

@sadielbartholomew
Copy link
Member

sadielbartholomew commented Nov 16, 2023

Thanks @JonathanGregory. In which case, I will merge the PR now since both yourself and Lars are happy for me to do so and as you point out, we have no objections.

Once we have completed this release, we ought to think a bit about the rules which should apply for accepting minor changes.

In my opinion it would indeed be useful to do this. Though I would point out that a proposal, however major or minor, may be implemented in a way that is or isn't satisfactory, so ideally the new rules could include the role of review of the PR(s) corresponding to the approved Issue such that merging is then allowed. For example, if one assigned reviewer (who is suitably informed e.g. understands the background and code details) gives formal approval then it can be merged, or similar. Some projects like two, and sometimes each is given a different focus or one does a thorough review and the other just a simple sanity check after that. (There may already be rules such as this, though I am not aware of them.)

@larsbarring larsbarring added this to the 1.11 milestone Nov 21, 2023
@larsbarring larsbarring added the change agreed Issue accepted for inclusion in the next version and closed label Nov 21, 2023
@larsbarring
Copy link
Contributor Author

I am closing this as completed via #476

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change agreed Issue accepted for inclusion in the next version and closed defect Conventions text meaning not as intended, misleading, unclear, has typos, format or language errors
Projects
None yet
Development

No branches or pull requests

5 participants