[skip changelog] Update contributions guidelines and PR template to add labeling and increase focus on breaking changes #1039

rsora · 2020-10-19T15:53:46Z

Please check if the PR fulfills these requirements

The PR has no duplicates (please search among the Pull Requests
before creating one)
The PR follows
our contributing guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce?

This PR updates the Contribution guidelines and the PR template to ask contributors and maintainers to use the label [breaking] when a breaking change is introduced.
Our internal release procedures are updated accordingly as well in order to:
- Extend the QA window we have between the release candidate creation and the actual release.
- Highlight better breaking changes in the changelog if any
- Created a process to handle breaking changes by creating migration guides in the UPGRADING.md file

Does this PR introduce a breaking change, and is labeled accordingly?

No.

Other information:

See how to contribute

silvanocerza · 2020-10-20T16:22:44Z

What should be considered a breaking change though? I think we should define that to know exactly when to mark a certain PR with that label.

Also wouldn't it better if the label is an actual issue label instead of text "polluting" the PR title?

per1234 · 2020-10-21T01:53:56Z

.github/PULL_REQUEST_TEMPLATE.md

@@ -16,7 +16,8 @@
 * **What is the new behavior?**
 <!-- if this is a feature change -->

- **Does this PR introduce a breaking change?**
+- **Does this PR introduce a breaking change, and is
+[labeled accordingly](https://arduino.github.io/arduino-cli/latest/CONTRIBUTING/#pull-requests)?**


I wonder it the term "labeled" is clear enough? It might lead people to think of GitHub's labeling feature.

I have two ideas of how things could be made more clear:

Change the term to something like "titled".

Add an anchor to the specific bullet point in the contributing guidelines, and link to that:
- <a id="breaking"></a>Please start the commit message and PR title with the string **[breaking]** if the PR contains a breaking change. A

per1234 · 2020-10-21T01:58:17Z

docs/CONTRIBUTING.md

@@ -54,6 +54,8 @@ submitting a PR:
 - PR titles indirectly become part of the CHANGELOG so it's crucial to provide a good record of **what** change is being
  made in the title; **why** it was made will go in the PR description, along with a link to a GitHub issue if it
  exists.
+- Please start the commit message and PR title with the string **[breaking]** if the PR contains a breaking change. A


Suggested change

- Please start the commit message and PR title with the string **[breaking]** if the PR contains a breaking change. A

- If the PR contains a breaking change, please start the commit message and PR title with the string **[breaking]**. A

I think this wording improves readability because it provides the condition up front, so it's easier to understand the purpose of the bullet point when skimming the document. That makes it easier for those who need it to find, and faster for those who don't need it to skip over.

per1234 · 2020-10-21T02:24:38Z

Also wouldn't it better if the label is an actual issue label instead of text "polluting" the PR title?

I do sometimes get a bit annoyed by the "pollution" of the titles with these labels. It makes it more difficult to write a meaningful title that fits in the character limit. However, I think this "pollution" is worth its weight in precious characters.

The labeling is a great idea, but we must keep in mind that community contributors can't label their PRs. I haven't seen it with PRs, but I do see periodically see comments from people submitting issues who are concerned because they wanted to follow the convention they see on the other issues of adding labels, but can't figure out how to do it (because it's impossible for them). So it would need to be clearly communicated. It becomes more of a maintainer guideline at that point than a contributor guideline

per1234 · 2020-10-21T02:26:49Z

I forgot to add that I investigated whether GitHub's pull request template system allows automatically labeling PRs according to front matter metadata, like the issue template system does, but it seems this is not supported.

obra · 2020-10-28T07:25:52Z

Over on #1044, @rsora suggested that I chime in here.

In my eyes, a "breaking" change is a one that intentionally makes the user change their code in order to keep working.

I believe that any PR for a breaking change should include an entry in an "UPGRADING" file that's separate from the main Changelog. That entry should include instructions telling a smart (but possibly uneducated) user what they need to do differently, ideally with an example and a note about which version of the tool the functionality was removed in.

In a perfect world, breaking changes should include code to warn the user when they're using the old, deprecated functionality. Ideally, the warning message should be a concise version of the UPGRADING entry and should tell the user exactly what to change to make their old code go.

Maybe the question for this template to ask is something like:

"If this PR is merged, will any users need to change their code, command-line invocations, build scripts or data files when upgrading from an older version of arduino-cli?"

rsora · 2020-11-05T11:05:14Z

docs/CONTRIBUTING.md

@@ -54,6 +54,10 @@ submitting a PR:
 - PR titles indirectly become part of the CHANGELOG so it's crucial to provide a good record of **what** change is being
  made in the title; **why** it was made will go in the PR description, along with a link to a GitHub issue if it
  exists.
+- <a id="breaking"></a>A breaking change is a change that forces users to change their code, command-line invocations,


@silvanocerza

What should be considered a breaking change though?

I added a better description to to both the PR template and the docs borrowing words from @obra (Thanks 😸 ). You like it?

@per1234 Nice suggestion about adding the anchor 👍 Please review the new wording like only you can do 🕵️

Yep, now it's clear what is meant.

rsora · 2020-11-05T11:08:34Z

DDoS countermeasures cause CI to fail
I'll open a separate PR after a thorough arduino.cc links checks

rsora · 2020-11-05T11:49:04Z

@obra I very like your suggestion:

I believe that any PR for a breaking change should include an entry in an "UPGRADING" file that's separate from the main Changelog.

As a user, where would you expect to see this file?

bundled with the binary?
as a Github Release attachment?

Having it as a section of the Release changelog we put in the release description could be easier for us to maintain and immediately available to users, as they can see it when they download the binary from the release page. OTOH we have other distribution channels for the CLI, in that case, maybe a file bundled with the release could reach the majority of our users.
I would love to have any hint from you on this 😺

.github/PULL_REQUEST_TEMPLATE.md

docs/CONTRIBUTING.md

per1234

Great work @rsora!

…dd labeling and increase focus on breaking changes

… request template

obra · 2020-11-07T07:52:39Z

As a user, where would you expect to see this file?

bundled with the binary?

as a Github Release attachment?

Having it as a section of the Release changelog we put in the release description could be easier for us to maintain and immediately available to users, as they can see it when they download the binary from the release page. OTOH we have other distribution channels for the CLI, in that case, maybe a file bundled with the release could reach the majority of our users.
I would love to have any hint from you on this 😺

Sorry for my slow reply.

One of the cool things about arduino-cli is that it's a self-contained executable and may well be distributed unpackaged. Because of that, I don't think a bundled file names sense.

A pattern that's somewhat common is an "UPGRADING" file in the repository containing a running log of breaking changes, but especially for a project with nice docs, having it on the documentation website is also reasonable, https://arduino.github.io/arduino-cli/ seems like a pretty natural place to have an "Upgrading" section.

I think that putting the content on the GitHub releases page isn't great. That's where you go to get the release, not where to learn about it. It's also not a place a PR making a breaking change can add content to ;) I'm a big fan of having the documentation about the breaking change be included in the PR that makes the change.

rsora · 2020-11-12T18:09:23Z

especially for a project with nice docs, having it on the documentation website is also reasonable, https://arduino.github.io/arduino-cli/ seems like a pretty natural place to have an "Upgrading" section.

This is an excellent Idea @obra, this should be compatible with our release workflow and also with how our documentation release process works!

Having different documentation "versions" bound to each release and each release bound to a "release branch", for each release that contains breaking changes it should be sufficient to add the proper changes to the "upgrading" docs section as a commit on the related release branch.

@per1234 WDYT? It sounds to me that this could be the right tradeoff in terms of maintainability for the team.

per1234 · 2020-11-13T02:51:33Z

an "UPGRADING" file that's separate from the main Changelog.

I don't see any benefit to having separate documents for the changelog and the migration guide. This information is fundamentally related.

A pattern that's somewhat common is an "UPGRADING" file in the repository containing a running log of breaking changes, but especially for a project with nice docs, having it on the documentation website is also reasonable,

The documentation content is hosted in the repository, so we would get both.

It's also not a place a PR making a breaking change can add content to ;) I'm a big fan of having the documentation about the breaking change be included in the PR that makes the change.

I very much agree with this sentiment. I would suggest just having a link from the releases page to that release's section of the changelog documentation page.

for each release that contains breaking changes it should be sufficient to add the proper changes to the "upgrading" docs section as a commit on the related release branch.

I think we should have a running document with a section for each release. The updates to the document should be committed to the master branch, then the release branch and the versioned documentation gets the snapshot of the file at that point in the history. This is how it's done in the Arduino IDE's repo:
https://github.com/arduino/Arduino/blob/master/build/shared/revisions.txt

The changelog and and any migration guides that weren't added to the changelog along with the PR that made the breaking change can be submitted as a PR to the repository before making the release.

silvanocerza · 2020-12-02T12:20:19Z

I talked with @rsora and we reached a conclusion for this.

We updated our internal release process to handle the release containing breaking changes.

Also we updated the contributing guidelines with new steps that require the contributors to edit the UPGRADING.md file whenever a breaking change is introduced with a PR they personally create following the suggestions from @obra. Thanks!

This will help us keep track of breaking changes and the user to handle them accordingly when a new version is released.

@per1234 may I ask you for a new review? Also could you take a look to the newly updated internal release process?

For the record we are discussing internally to publish our release process guidelines.

Don't be bothered about the verify-links check failing, it's complaining that it can't find the new upgrading page on the documentation but that's normal until we merge this PR.

per1234 · 2020-12-02T16:30:47Z

docs/CONTRIBUTING.md

@@ -421,3 +426,4 @@ If your PR doesn't need to be included in the changelog, please start the commit
 [npm-install-docs]: https://docs.npmjs.com/downloading-and-installing-node-js-and-npm
 [poetry-website]: https://python-poetry.org/
 [poetry-docs]: https://python-poetry.org/docs/
+[upgrading-file]: ../UPGRADING


Suggested change

[upgrading-file]: ../UPGRADING

[upgrading-file]: UPGRADING.md

This allows the link to work when viewing the documentation via the GitHub web interface, which is especially likely with CONTRIBUTING.md, since GitHub prompts contributors to read it there.

per1234 · 2020-12-02T16:32:22Z

docs/UPGRADING.md

+
+Here you can find a list of migration guides to handle breaking changes between releases of the CLI.
+
+For more information about breaking changes see the [contributing documentation][contrib-docs].


I don't think the linked information is of interest to the target reader of this file, so I would just remove this sentence.

If it is to be retained, please change the link to be relative:

[contrib-docs]: CONTRIBUTING.md#breaking

Right, didn't think about that. I'll just remove it.

per1234 · 2020-12-02T16:34:16Z

mkdocs.yml

  - getting-started.md
  - command-line-completion.md
  - CONTRIBUTING.md
+  - UPGRADING.md


Suggested change

- getting-started.md

- command-line-completion.md

- CONTRIBUTING.md

- UPGRADING.md

- UPGRADING.md

- getting-started.md

- command-line-completion.md

- CONTRIBUTING.md

It seems better placed in the index directly under "Installation".

per1234 · 2020-12-02T16:41:43Z

docs/UPGRADING.md

+
+## Unreleased
+
+[contrib-docs]: https://arduino.github.io/arduino-cli/latest/CONTRIBUTING/#breaking


Suggested change

[contrib-docs]: https://arduino.github.io/arduino-cli/latest/CONTRIBUTING/#breaking

This is no longer needed.

Right, I forgot. 🤦

rsora requested a review from per1234 October 19, 2020 16:07

per1234 requested changes Oct 21, 2020

View reviewed changes

rsora mentioned this pull request Oct 27, 2020

Deprecate compile --build-properties flag in favor of new featureful --build-property flag #1044

Merged

4 tasks

rsora force-pushed the rsora/breaking-contribution branch from a80fa54 to 7b886cb Compare November 5, 2020 10:43

rsora commented Nov 5, 2020

View reviewed changes

per1234 requested changes Nov 5, 2020

View reviewed changes

.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved

docs/CONTRIBUTING.md Outdated Show resolved Hide resolved

per1234 approved these changes Nov 5, 2020

View reviewed changes

rsora added 2 commits November 5, 2020 15:21

[skip changelog] Update contributions guidelines and PR template to a…

749ee33

…dd labeling and increase focus on breaking changes

Add better explanation of breaking changes based on suggestions

b1fd6c0

rsora force-pushed the rsora/breaking-contribution branch from 076d964 to b1fd6c0 Compare November 5, 2020 14:21

rsora added 2 commits November 5, 2020 15:27

Reword breaking change paragraph in docs and Arduino CLI name in pull…

edb4bc3

… request template

Revert typo

8150414

silvanocerza assigned rsora Dec 2, 2020

[skip changelog] Update breaking changes policy

f38f1d2

silvanocerza requested a review from per1234 December 2, 2020 12:20

per1234 requested changes Dec 2, 2020

View reviewed changes

[skip changelog] Fix some docs issues

d9bcfbf

per1234 requested changes Dec 2, 2020

View reviewed changes

[skip changelog] Remove unused link from docs

9f7f31c

per1234 approved these changes Dec 2, 2020

View reviewed changes

silvanocerza merged commit d0e904c into master Dec 2, 2020

silvanocerza deleted the rsora/breaking-contribution branch December 2, 2020 16:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[skip changelog] Update contributions guidelines and PR template to add labeling and increase focus on breaking changes #1039

[skip changelog] Update contributions guidelines and PR template to add labeling and increase focus on breaking changes #1039

rsora commented Oct 19, 2020 •

edited by silvanocerza

Loading

silvanocerza commented Oct 20, 2020

per1234 Oct 21, 2020

per1234 Oct 21, 2020

per1234 commented Oct 21, 2020

per1234 commented Oct 21, 2020

obra commented Oct 28, 2020

rsora Nov 5, 2020

silvanocerza Nov 5, 2020

rsora commented Nov 5, 2020 •

edited

Loading

rsora commented Nov 5, 2020 •

edited

Loading

per1234 left a comment

obra commented Nov 7, 2020

rsora commented Nov 12, 2020

per1234 commented Nov 13, 2020

silvanocerza commented Dec 2, 2020

per1234 Dec 2, 2020

per1234 Dec 2, 2020

silvanocerza Dec 2, 2020

per1234 Dec 2, 2020

per1234 Dec 2, 2020

silvanocerza Dec 2, 2020

	- Please start the commit message and PR title with the string [breaking] if the PR contains a breaking change. A
	- If the PR contains a breaking change, please start the commit message and PR title with the string [breaking]. A

	[upgrading-file]: ../UPGRADING
	[upgrading-file]: UPGRADING.md


		Here you can find a list of migration guides to handle breaking changes between releases of the CLI.

		For more information about breaking changes see the [contributing documentation][contrib-docs].


		## Unreleased

		[contrib-docs]: https://arduino.github.io/arduino-cli/latest/CONTRIBUTING/#breaking

[skip changelog] Update contributions guidelines and PR template to add labeling and increase focus on breaking changes #1039

[skip changelog] Update contributions guidelines and PR template to add labeling and increase focus on breaking changes #1039

Conversation

rsora commented Oct 19, 2020 • edited by silvanocerza Loading

silvanocerza commented Oct 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

per1234 commented Oct 21, 2020

per1234 commented Oct 21, 2020

obra commented Oct 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsora commented Nov 5, 2020 • edited Loading

rsora commented Nov 5, 2020 • edited Loading

per1234 left a comment

Choose a reason for hiding this comment

obra commented Nov 7, 2020

rsora commented Nov 12, 2020

per1234 commented Nov 13, 2020

silvanocerza commented Dec 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsora commented Oct 19, 2020 •

edited by silvanocerza

Loading

rsora commented Nov 5, 2020 •

edited

Loading

rsora commented Nov 5, 2020 •

edited

Loading