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

Retire stale changewaves #10404

Merged
merged 8 commits into from
Jul 30, 2024
Merged

Retire stale changewaves #10404

merged 8 commits into from
Jul 30, 2024

Conversation

rainersigwald
Copy link
Member

After discussion in #9543 we decided to delay changewave retirement to a release corresponding with 9.0.100. The time is now.

@rainersigwald
Copy link
Member Author

Pinging folks relevant to the changes that are being made permanent: @AR-May, @JanKrivanek, @Forgind, @JaynieBai.

Also @marcpopMSFT as a heads up.

@rainersigwald rainersigwald self-assigned this Jul 19, 2024
@rainersigwald rainersigwald requested a review from a team July 19, 2024 14:05
Copy link
Member

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

I don't think this is actually necessary, but the example in ChangeWaves-Dev.md uses Wave17_4; we could update that to 17_12 or something.

Also, I thought we were planning to always have three change waves active at a time? If we're changing that, perhaps we should update the .md to say that 🙂

Finally, I have no idea why, but apparently this is a thing?

<data name="PropertyOutsidePropertyGroupInTarget" xml:space="preserve" Condition="$([MSBuild]::AreFeaturesEnabled('17.6'))">

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Overall looks good.
Before signing off it'd be nice to know more details about code being removed that's not part of changewaves condition on a first look (I know big portion of that is just port from the previous PR - but I didn't see the context there either)

@rainersigwald
Copy link
Member Author

Also, I thought we were planning to always have three change waves active at a time? If we're changing that, perhaps we should update the .md to say that 🙂

I interpreted

A wave of features is set to "rotate out" (i.e. become standard functionality) two bands after its release. For example, wave 16.8 stayed opt-out through wave 16.10, becoming standard functionality when wave 17.0 is introduced.

and

Invalid Value (Ex: 16.9 when valid waves are 16.8 and 16.10)

To mean we should generally only have two available but I couldn't remember the details on this part. @marcpopMSFT do you, by chance?

@Forgind
Copy link
Member

Forgind commented Jul 29, 2024

Also, I thought we were planning to always have three change waves active at a time? If we're changing that, perhaps we should update the .md to say that 🙂

I interpreted

A wave of features is set to "rotate out" (i.e. become standard functionality) two bands after its release. For example, wave 16.8 stayed opt-out through wave 16.10, becoming standard functionality when wave 17.0 is introduced.

and

Invalid Value (Ex: 16.9 when valid waves are 16.8 and 16.10)

To mean we should generally only have two available but I couldn't remember the details on this part. @marcpopMSFT do you, by chance?

It seems a bit ambiguous. I saw examples like this:

public static readonly Version[] AllWaves = { Wave17_0, Wave17_2, Wave17_4 };

which has three active change waves after the update. I interpreted two bands after its release to mean that we'd have 17_0 and 17_2 would be released (and active), and 17_4 would be active but not released (because it's in preview). Then when 17_4 releases, 17_0 would rotate out, and 17_6 would become the new preview version.

Regardless, I don't think it actually matters, so I have no problem with your interpretation and making that the official version.

@rainersigwald
Copy link
Member Author

Ah; I think that makes sense in e.g. the 17.3 release--but now we're in the 17.12 release so that one will be "released and active", right?

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Let's slash that code

@Forgind
Copy link
Member

Forgind commented Jul 30, 2024

Ah; I think that makes sense in e.g. the 17.3 release--but now we're in the 17.12 release so that one will be "released and active", right?

I'd say we're working on 17.12, so it's active but still in preview, that is, not released.

@rainersigwald rainersigwald merged commit dcd7772 into dotnet:main Jul 30, 2024
10 checks passed
@rainersigwald rainersigwald deleted the stale🌊 branch July 30, 2024 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants