Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add a new template to show the list of host functions #7158

Merged
merged 3 commits into from
May 8, 2023

Conversation

chevdor
Copy link
Contributor

@chevdor chevdor commented May 2, 2023

Before

image

After

image

@chevdor chevdor requested a review from a team as a code owner May 2, 2023 09:40
@chevdor chevdor added A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes T8-release This PR/Issue is related to topics touching the release notes. labels May 2, 2023
@paritytech-ci paritytech-ci requested a review from a team May 2, 2023 09:41
@chevdor chevdor requested a review from ggwpez May 2, 2023 09:41
@paritytech-ci paritytech-ci requested a review from a team May 2, 2023 09:47
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Thanks for fixing!

@koute your MR actually modified a host function, not added a new one, or?
So we should probably change the description of the label and section eventually.

@chevdor
Copy link
Contributor Author

chevdor commented May 2, 2023

@ggwpez @koute expained actually quite nicely in his PR.

This situation occurs quite often: a new host function is added but not in use yet.
So setting the label is legit but reporting it as we could be seen as a false "alarm" since it is not in use.

Still it is better to raise awareness that a new host function showed up. If users upgrade now, it will be all good if they don't later when it is really needed.

We could add a special mention as free note but is it worth it ?

@ggwpez
Copy link
Member

ggwpez commented May 2, 2023

I think it is fine, my concern was about the formulation: "one new host function".
But there was no new one - it was just modifying an existing one (actually two) by bumping a dependency.

@bkchr
Copy link
Member

bkchr commented May 2, 2023

But there was no new one - it was just modifying an existing one (actually two) by bumping a dependency.

Exactly this is correct!

It is then required by a human to "fix" this afterwards in the release notes.

@chevdor chevdor mentioned this pull request May 2, 2023
15 tasks
@chevdor
Copy link
Contributor Author

chevdor commented May 3, 2023

I am editing this for the upcoming release and indeed I should change the wording in the template.

The number we inject in the template is not the number of host functions as you pointed out @ggwpez but the number of PRs related to host functions.

The E3 label has been mostly used to introduce new host functions (active or not) but the labels do not carry the information about the number of host fn affected.
Now with the list of related PRs, we can see a bit more details about those changes.

I will rephrase that part.

@chevdor chevdor marked this pull request as draft May 3, 2023 09:36
@koute
Copy link
Contributor

koute commented May 8, 2023

@koute your MR actually modified a host function, not added a new one, or? So we should probably change the description of the label and section eventually.

Yes. Would be nice to have a dedicated label for this. Maybe something like this:

  • one label for when we introduce a new host function (host function is not used yet, so no need for everyone to upgrade; low priority)
  • one label for when we enable a previously unused host function (everyone has to upgrade; high priority)
  • one label for when we modify a previously unused but already existing in in-the-wild host function (so that if we enable it we know what is the oldest version of node that's still compatible)

cc @the-right-joyce

@chevdor
Copy link
Contributor Author

chevdor commented May 8, 2023

I am totally fine with that @koute. This kind of granularity allows being super accurate when listing/grouping those PRs.
This will however only work if those extra labels are used properly by everyone and picking the right set of labels has an overhead.
We have already lots of labels and I know the amount if already daunting. That being said, the number of people working around host functions is limited so it may work.

For now, I will fix the templates with the labels we currently have. @koute I opened a separate issue to keep track of your suggestion.

@chevdor
Copy link
Contributor Author

chevdor commented May 8, 2023

Here is a preview of the new change in the case of 1 PR related to host functions:
image

@chevdor chevdor marked this pull request as ready for review May 8, 2023 09:16
@chevdor
Copy link
Contributor Author

chevdor commented May 8, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@chevdor
Copy link
Contributor Author

chevdor commented May 8, 2023

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes T8-release This PR/Issue is related to topics touching the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants