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

Release - 1.2.6 (new PR #3353) #3351

Closed
wants to merge 6 commits into from
Closed

Release - 1.2.6 (new PR #3353) #3351

wants to merge 6 commits into from

Conversation

nivida
Copy link
Contributor

@nivida nivida commented Jan 30, 2020

Description

This release will be done without the release of an RC version and the defined test period of one week because of the importance changing of the ENS Registry address does have.

Release Guideline

Added

Changed

Fixed

Compare View

v1.2.5 -> v1.2.6

Type of change

  • Release

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.
  • I ran npm run test with success and extended the tests if necessary.
  • I ran npm run build and tested the resulting file from dist folder in a browser.
  • I have tested the minified file locally

@nivida nivida added Release 1.x 1.0 related issues labels Jan 30, 2020
@nivida nivida requested a review from cgewecke January 30, 2020 13:56
@nivida nivida added the In Progress Currently being worked on label Jan 30, 2020
@coveralls
Copy link

coveralls commented Jan 30, 2020

Coverage Status

Coverage remained the same at 85.466% when pulling 1ccfe27 on release/1.2.6 into a121b6f on 1.x.

@alcuadrado
Copy link

I think it would be better to release 1.2.6 with only the ENS addresses changes. This minimizes the chance of breaking something else, while still updates the ENS registry.

Copy link
Collaborator

@cgewecke cgewecke left a comment

Choose a reason for hiding this comment

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

@nivida I agree with @alcuadrado

1.2.6 should be the smallest difference from 1.2.5 which will work. We should honor the ideas behind the release process as closely as possible to reduce risk.

Copy link
Contributor

@michaelsbradleyjr michaelsbradleyjr left a comment

Choose a reason for hiding this comment

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

ENS changes look good!

One question: when I look at the v1.2.5...release/1.2.6 comparison, there seem to be more changes than listed in the Added...Changed...Fixed lists above. So is the intention to release with only the commits corresponding to those lists or all the commits seen in the comparison view?

@nivida
Copy link
Contributor Author

nivida commented Jan 30, 2020

@alcuadrado @cgewecke

1.2.6 should be the smallest difference from 1.2.5 which will work. We should honor the ideas behind the release process as closely as possible to reduce risk.
I think it would be better to release 1.2.6 with only the ENS addresses changes. This minimizes the chance of breaking something else, while still updates the ENS registry.

I see the point but do not agree with it in this case. Because we do have time to check those small improvements which will definitely not introduce a breaking change are we able to release them by side of the changed addresses. I really appreciate to be as aligned as possible with the Release Guidlines we do have defined but I'm the meaning we shouldn't exaggerate.

@michaelsbradleyjr

ENS changes look good!

👌

v1.2.5...release/1.2.6

About what changes do you talk about? The CHANGELOG got updated always and there shouldn't be any missing point.

@cgewecke
Copy link
Collaborator

cgewecke commented Jan 30, 2020

@nivida

we shouldn't exaggerate

I can see the point you're making, although I think it's easier to have confidence that everything works if you contribute to the project every day.

The release protocol makes sure the broader community has a chance to evaluate how they might be affected by changes. Its goal is to bridge the gap between what they know about their code and what the maintainers here believe is ok.

Keeping the change-set as small as possible vs release/1.2.5 seems reasonable and should not impose undue burdens on this release.

Why not be extra-cautious here - even if it seems unnecessary - esp. since some post 1.2.5 changes directly affect the ENS module?

@nivida
Copy link
Contributor Author

nivida commented Jan 30, 2020

@cgewecke Thanks for your input and to explain your thoughts.

it's easier to have confidence

I wouldn't say it is just confidence because our test cases do cover enough to be sure anything does work as it was working before. The changes we have applied to the code base do not remove any single line they just add optional additional logic which can be released as a patch release. By side of it did we improve our PR review process extremely to be sure anything which does get merged will not introduce a breaking change.

Keeping the change-set as small as possible vs release/1.2.5 seems reasonable...

I think this isn't something which we should see as an unwritten rule. Let us define exactly the rules for the changeset for a normal release and what exactly can be included in an "emergency case" as we have now. This would probably also be a good point to add different emergency levels and the related escalation rules to it.

Why not be as cautious as possible here - even if it seems unnecessary - esp. since some post 1.2.5 changes directly affect the ENS module?

Because the changes do definitely not introduce a breaking change and we reviewed them extensively. Those are clearly changes that can be released with a patch release without any further steps.

@cgewecke
Copy link
Collaborator

Let us define exactly the rules for the changeset for a normal release and what exactly can be included in an "emergency case" as we have now. This would probably also be a good point to add different emergency levels etc. and the related escalation rules to it.

Yes, agree. Unfortunately as often happens what to do in emergencies is discovered in an emergency.

Even though you are likely correct about the current 1.x branch being publishable, it still seems like the most conservative thing to do is print 1.2.5 plus address changes, (maybe plus the goerli address as in #3338?)

I don't think your assessment of the safety of 1.x is simply "wrong" or something - just that there is a lower-risk alternative which is also available and that's what looks closest to the ideas expressed by the release/review guidelines.

@holgerd77
Copy link
Collaborator

I also would pledge here for the minimal version, no need here to break out here from the defined new guidelines, this will unnecessarily lower the trust in the process.

@michaelsbradleyjr
Copy link
Contributor

@nivida

About what changes do you talk about?

I looked again, and it seems that the other commits in the comparison mostly involve changes to tests/docs, so I was mistaken that something was left out of the Added...Changed...Fixed lists.

@nivida
Copy link
Contributor Author

nivida commented Jan 31, 2020

@michaelsbradleyjr 👌 (this confusion is a clear argument to introduce conventional commits as once mentioned by you)

It is probably worth to create a Release Process Improvements issue out of the discussions we had and to list all required improvements up and to be open for further feedback from the community and especially from the contributors of the web3.js project.

Now to come back to the actual topic we have. Instead of proceeding the discussion is it probably better to list up the facts we have and to decide based on those.


Release of the already proposed PR

Violated Release Rules: 1, 2, 3, and 4 (with exception of the last sentence)
Violated PR Rules: 2

Other Disadvantages


Proposed release from the reviewers

Violated Release Rules: 1, 2, 3, and 4 (with exception of the last sentence)
Violated PR Rules: 2
Violated Review Rules: 2

Other Disadvantages

  • Release PR can't get merged back into 1.x (base head of the release branch will be behind)
  • The time until the new addresses do get published will get increased

It would be interesting to find out what violations should weigh how heavy and how we handle the listed disadvantages in the future.

I will publish it tomorrow and do what we have decided (CEST).
@holgerd77 @cgewecke @alcuadrado @JosefJ

Edit:
My vote is to release the current PR because of the emergency case it is and the QA we have.

Release of the already proposed PR: 🚀
Proposed release from @cgewecke: : ❤️

@holgerd77
Copy link
Collaborator

@nivida Thanks for the structured writeup and summary, really great to have some in-between analysis here. I am a bit in doubt if counting the number of violated rules is a fair comparison here, since a single non-RC-channeled change atm just violates as much rules as all additional arbitrary number of changes. 😛

I think we just have a new and additional case here which hasn't been included in the release process definition yet. So I think it's a good idea to open a new Release Process Improvements issue and add rules for such an emergency case and eventually also discuss other improvement ideas coming up.

@holgerd77
Copy link
Collaborator

@nivida Really love the icons you've got chosen for the vote, very intuitive. 😋

@nivida nivida closed this Jan 31, 2020
@nivida nivida deleted the release/1.2.6 branch January 31, 2020 09:54
@nivida nivida removed the In Progress Currently being worked on label Jan 31, 2020
@nivida nivida changed the title Release - 1.2.6 Release - 1.2.6 (new PR #3353) Jan 31, 2020
@nivida
Copy link
Contributor Author

nivida commented Jan 31, 2020

I've opened #3353 which does show clearly the discrepancy between 1.x -> release branch and I've changed the title of this PR to forward a user to the correct PR for the release.

@michaelsbradleyjr Thanks for your vote but I've followed the recommendations from the overall team lead of the EF JS Team @holgerd77 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues Release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants