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

CLIENT-SPECIFICATION: v1.5 #5428

Merged
merged 4 commits into from
Mar 19, 2021
Merged

CLIENT-SPECIFICATION: v1.5 #5428

merged 4 commits into from
Mar 19, 2021

Conversation

bl-ue
Copy link
Contributor

@bl-ue bl-ue commented Mar 12, 2021

Also see: #5377 (comment). Can we do it?

@bl-ue bl-ue added documentation Issues/PRs modifying the documentation. clients Issues pertaining to a particular client or the clients as whole. labels Mar 12, 2021
@bl-ue bl-ue requested review from MasterOdin and mebeim March 12, 2021 18:17
@MasterOdin
Copy link
Collaborator

No matter what, you're going to need two commits to update the changelog appropriately with the link to the commit. As such, I did purposely leave out the version bump in #5327 to be done in a follow-up PR as I'd prefer the commit history of:

  • CLIENT-SPECIFICATION: some change
  • CLIENT-SPECIFICATION: v1.5

versus

  • CLIENT-SPECIFICATION: some change
  • CLIENT-SPECIFICATION: fix changelog

@bl-ue
Copy link
Contributor Author

bl-ue commented Mar 12, 2021

No matter what, you're going to need two commits to update the changelog appropriately with the link to the commit.

Yeah, I know that and I don't see any way around it :\

I did purposely leave out the version bump in #5327 to be done in a follow-up PR

Ah, I didn't know that. Thank you.

I agree with you about the commit format. I thought the same before I opened this PR 👍🏻

I'd prefer the commit history of:

  • CLIENT-SPECIFICATION: some change
  • CLIENT-SPECIFICATION: v1.5

versus

  • CLIENT-SPECIFICATION: some change
  • CLIENT-SPECIFICATION: fix changelog

So @MasterOdin are you thinking to, in the changelog, link to the last PR that modified the version (#5327, for v1.5), or the PR that cut the release (#5428 (this PR), for v1.5)?

In case of the former, we should update the changelog now, in this PR. In case of the latter, which makes more sense, we'll need a 'fix changelog' commit which isn't ideal.... 🤔

CLIENT-SPECIFICATION.md Outdated Show resolved Hide resolved
Co-authored-by: Starbeamrainbowlabs <sbrl@starbeamrainbowlabs.com>
@bl-ue
Copy link
Contributor Author

bl-ue commented Mar 13, 2021

Before we merge this, see this: #5184 (comment). Should we perhaps wait for this issue to be resolved before merging this?

Copy link
Member

@mebeim mebeim left a comment

Choose a reason for hiding this comment

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

Waiting for #5464, then we can update the changelog permalink while also mentioning the HTTPS change and merge this. Also: please squash when merging.

@mebeim
Copy link
Member

mebeim commented Mar 17, 2021

I have updated the changelog linking v1.5 to my last commit, but I think the current update flow is not really ideal. The link of v1.5 now points to 8505e55, but there the file says "Current Specification Version: 1.4", which is confusing.

We should adopt the following flow for updating the client specification:

  1. Keep the latest version linked to /blob/master in the changelog.
  2. When we need a new version, open a PR (or even a draft PR) to start the work.
  3. Do all the changes needed.
  4. Finally add final commit that only bumps the version, adding a new link to /blob/master and replacing the previous one with /blob/commit_hash_of_previous_version_bump.

This way links will always be correct, and also there ideally is no time span in which we have an inconsistent state of the client spec (like for example right now, we have changes already incorporated but the spec says v1.4, not that cool).

What do you all think?

EDIT: I'll add this as a maintainer's note in the file for future reference.

@bl-ue
Copy link
Contributor Author

bl-ue commented Mar 18, 2021

@mebeim we could also link to the release, instead of the commit (and cut the release immediately after merging so that the commit the GH automatically links to the release is the merge commit of this PR), something like this:

- - [v1.5, March 17th 2021](https://github.com/tldr-pages/tldr/blob/034111a547ec3e1e2a7a292de17b970cdf77288e/CLIENT-SPECIFICATION.md) ([#5464](https://github.com/tldr-pages/tldr/pull/5464))
+ - [v1.5, March 17th 2021](https://github.com/tldr-pages/tldr/releases/tag/v1.5))
    - Add requirement for converting command names to lowercase before running the page resolution algorithm.
    - Use HTTPS for archive links.

(e.g. https://github.com/tldr-pages/tldr-python-client/releases/tag/1.2.0)

@mebeim
Copy link
Member

mebeim commented Mar 18, 2021

@bl-ue not sure if that's a better idea, as it requires a lot more manual work, i.e. creating the tag, pushing it, creating a release, creating an artifact for the release adding it, and then linking to that. I'd want the link to point to the actual document rather than the release.

@bl-ue
Copy link
Contributor Author

bl-ue commented Mar 18, 2021

@mebeim sorry, I wasn't clear: you can link to files using the release tag, e.g. https://github.com/tldr-pages/tldr-python-client/blob/1.2.0/tldr.py (notice 1.2.0, that's this release's tag)

@bl-ue
Copy link
Contributor Author

bl-ue commented Mar 18, 2021

The benefit of that approach is that you don't have to wait for the release to be made; you can link to the release directly before it's even made because the URL is predictable.

@mebeim
Copy link
Member

mebeim commented Mar 18, 2021

@bl-ue Ohhh, I see. Then we can just create a tag and link to the tags like that (e.g. /blob/v1.5/... ). I think that makes sense!

@mebeim mebeim merged commit ba01b3a into master Mar 19, 2021
@mebeim mebeim deleted the bl-ue/client-spec-v1.5 branch March 19, 2021 21:58
@bl-ue
Copy link
Contributor Author

bl-ue commented Mar 19, 2021

Hmm...I didn't get notified about this. @mebeim I think we need to create an actual release, not just a tag 🤔

I actually get GH notifications when I'm watching a repository and they create a release. For example, I was notified when https://github.com/kubernetes/kubernetes/releases/tag/v1.20.5 was released.

@MasterOdin
Copy link
Collaborator

GH notifies on issues, pull requests, discussions, and releases. It does not notify on tags so far as I'm aware.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clients Issues pertaining to a particular client or the clients as whole. documentation Issues/PRs modifying the documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants