Skip to content
This repository has been archived by the owner on May 4, 2022. It is now read-only.

docs: add major version upgrade documentation #647

Merged
merged 3 commits into from
Dec 20, 2017

Conversation

davidmsibley
Copy link
Contributor


Contributor License Agreement adherence:

@@ -0,0 +1,62 @@
# Upgrading
Copy link
Contributor

Choose a reason for hiding this comment

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

+1. Breaking out this guidance about major version upgrades from the bulk of CHANGELOG is a great idea. It really emphasizes what there is to know about this upgrading (or if somehow we've overlooked something, what the gaps are). Big +1 from me, nicely done.

@davidmsibley
Copy link
Contributor Author

had to rewrite history because I pushed a commit message that started with "chore:" and that's apparently not valid.

I wish it would just assume that it's a minor version commit unless I tell it otherwise.

@vertein
Copy link
Contributor

vertein commented Dec 20, 2017

I could have sworn that I have successfully used chore: in the past. I have used it in uPortal-home and the checks liked it.

@ChristianMurphy
Copy link
Contributor

ChristianMurphy commented Dec 20, 2017

had to rewrite history because I pushed a commit message that started with "chore:" and that's apparently not valid.

I could have sworn that I have successfully used chore: in the past. I have used it in uPortal-home.

It used to be, Angular is in the process of changing their commit conventions, chore: is the first casualty.
Ref: https://github.com/marionebl/commitlint/issues/146

edit see ongoing conversation in #648

@ChristianMurphy
Copy link
Contributor

ChristianMurphy commented Dec 20, 2017

I wish it would just assume that it's a minor version commit unless I tell it otherwise.

That can be done with a new rule set for commitlint and/or semantic release.

The question is, is that really what is wanted?
Conventional commits are more than just versioning, they communicate scope an impact of a change at a glance.
Assuming minor version would save a few keystrokes for versioning, but leaves determining scope and impact to reading the source code.

@davidmsibley
Copy link
Contributor Author

The question is, is that really what is wanted?

I just don't think contributor commit messages is the right place for any of this. The closest I've come to agreeing with commit messages is using a commitlintbot to check the title of a pull request so that when the committer squashes and merges it, the commit message is correct in the canonical repo.

If we enforce it in contributor commit messages, I'd want one of two scenarios:

  1. The ONLY types allowed would be fix:, feat: and BREAKING CHANGE:
    or
  2. all commits are assumed to be minor version changes unless prefixed with feat: or BREAKING CHANGE:

The idea that we allow an additional arbitrary set of types that we require the contributor to know and classify their commits correctly is absurd. As a contributor, I don't trust myself to do it correctly, why should I trust anyone else? The committers role is to investigate the suggested changes and to correctly classify them.

@davidmsibley davidmsibley merged commit 7aeeba3 into uPortal-Attic:master Dec 20, 2017
@apetro
Copy link
Contributor

apetro commented Dec 20, 2017

Nitpick: I think there might be some conflating minor and patch version changes in the discussion above: feat: commits should always imply that the next release will at least bump the minor (not patch) part of the version. So maybe what @davidmsibley is expressing here is a desire that we assume commits are patch level changes (bugfixes) unless otherwise specified.

@davidmsibley
Copy link
Contributor Author

assume commits are patch level changes (bugfixes) unless otherwise specified

yup, sorry. I definitely meant patch, not minor.

@ChristianMurphy
Copy link
Contributor

The closest I've come to agreeing with commit messages is using a commitlintbot to check the title of a pull request so that when the committer squashes and merges it, the commit message is correct in the canonical repo.

I agree that tooling around this should continue to improve, and that is should be handled somehow in pull requests.
I'm cautious on saying PR title + Squash Merge is the best answer.
This works great for maintainers who have access to the squash an merge button.
But will erase history on non-committer contributors.
To me attribution of a change to the original author, using Git's built in conventions, is critical to maintain.

The idea that we allow an additional arbitrary set of types that we require the contributor to know and classify their commits correctly is absurd.

Poeple don't need to know the types, commitizen and commitlint prompt provide a UI that lists the types and descriptions, and allows them to be selected.

I also strong disagree with the idea that contributors needing to know can classify their commits is absurd.
To me, its absurd that: when a contributor doesn't understand how they are changing the code, we expect that maintainer to be able to look at the change, with no context on the intended scope and impact, and psychically know what the contributor meant.

There is an argument that this could be done via PR comments, but those comments happen hours, days or even weeks after the change is made, by the time the discussion starts the original contributor is thinking about a new problem, and is forgetting the context of the change in review.

As a contributor, I don't trust myself to do it correctly, why should I trust anyone else?

We don't expect people to handle commit messages perfectly.
I figure having imperfect context on intended impact and scope is better than having no context,
and that contributors classifications will improve as they gain experience.

The committers role is to investigate the suggested changes and to correctly classify them.

I agree that committers should investigate and help to classify commits correctly.
I disagree that the burden of review and classification fall solely on the shoulders of committers.
That's why I push for continuous integration and static analysis to help with the investigation process, and why I'm pushing for conventional commits to help with classification.
Contributors need to be aware of the process, and actively engaged in the process, so that slowly but surely they move toward a point where they could become committers.

@davidmsibley
Copy link
Contributor Author

we expect that maintainer to be able to look at the change, with no context on the intended scope and impact, and psychically know what the contributor meant.

We expect them to know what the impact of those changes will be on the existing codebase and the direction of development. The contributor can help them fix problems in the code by giving them context, but that's up to the contributor.

I disagree that the burden of review and classification fall solely on the shoulders of committers.

Like it or not, the buck stops at the committer. It ultimately doesn't matter how much effort and verification the contributor puts in, because the committer will need to assume the responsibility.

Contributors need to be aware of the process, and actively engaged in the process, so that slowly but surely they move toward a point where they could become committers.

That is perfectly achievable in many different ways (and often happens organically), it does not require commit message linting. Also, not all contributors want to become committers.

That's why I push for continuous integration and static analysis to help with the investigation process

Yes, CI and static analysis can help with that, but they are not acting as helpers for this project. They have been set up as gatekeepers. We require CI to pass before a pull request can be accepted. They literally prevent commits if their build fails. Ultimately this is why I push back so much on these linters and commit hooks: Every one of these adds an extraneous burden to contributing. These should be adopted cautiously and sparingly.

Here's my own personal opinion and then I'll drop this:
I don't like gatekeeping. I don't like dictating what tools developers use or how they work. In my mind, the only restrictions that are justifiable are ones that ensure the running code is better than it was before.

We shouldn't put restrictions on contributors because being a committer is hard. That's the job. Yes, we use tools to make the job easier, but we don't shift the burden.

@apetro
Copy link
Contributor

apetro commented Dec 20, 2017

Colleagues,

If this conversation needs to continue (and eventually I do think some form of it needs to continue to get to resolution), let's do it in a better context than as comments on a tangentially related Pull Request. uportal-dev@ list, on a uP developer call, as an ad-hoc Hangout, on a GitHub Issue focused on this very topic -- any of those please.

For whatever it's worth, I see merit in both your perspectives. I myself ended up abandoning documentation improvements I'd drafted because I couldn't then cope with getting linting to pass. And I'm grateful that recent MyUW commit messages trend way more thoughtful than some of the early commit messages - it made a real difference in my ability to analyze e.g. our uPortal mods.

I'll look forward to doing my part to continue to work through these tradeoffs -- in a better discussion context please. :)

-Andrew

@ChristianMurphy
Copy link
Contributor

let's do it in a better context than as comments on a tangentially related Pull Request. uportal-dev@ list, on a uP developer call, as an ad-hoc Hangout, on a GitHub Issue focused on this very topic -- any of those please.

deletes in progress response 😅
Relevant: there is a call for topics for the upcoming uPortal technical call https://groups.google.com/a/apereo.org/d/msg/uportal-dev/58B5eBNkLBQ/r5R_snGEDAAJ

@vertein vertein added this to the 8.0.0 milestone Jan 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants