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

feat(cascading): option to bypass non-latest minor branch #2473

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kpanot
Copy link
Contributor

@kpanot kpanot commented Nov 15, 2024

Proposed change

Option to bypass non-latest minor branch on cascading bot

Related issues

- No issue associated -

@kpanot kpanot requested a review from a team as a code owner November 15, 2024 03:33
Copy link

codecov bot commented Nov 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.31%. Comparing base (0042a21) to head (c4a10be).
Report is 10 commits behind head on main.

✅ All tests successful. No failed tests found.

Additional details and impacted files

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kpanot kpanot force-pushed the feature/cascading-last-minor branch from fd66a43 to e586aba Compare November 15, 2024 03:38
@kpanot kpanot force-pushed the feature/cascading-last-minor branch from e586aba to 5cecb54 Compare November 15, 2024 08:26
@kpanot kpanot force-pushed the feature/cascading-last-minor branch from 5cecb54 to 167e161 Compare November 15, 2024 08:30
fpaul-1A
fpaul-1A previously approved these changes Nov 15, 2024
mrednic-1A
mrednic-1A previously approved these changes Nov 15, 2024
@kpanot kpanot force-pushed the feature/cascading-last-minor branch from 167e161 to 63c73c0 Compare November 15, 2024 14:07
@kpanot kpanot dismissed stale reviews from mrednic-1A and fpaul-1A via ef7d65a November 15, 2024 14:19
@kpanot kpanot force-pushed the feature/cascading-last-minor branch from 63c73c0 to ef7d65a Compare November 15, 2024 14:19
cpaulve-1A
cpaulve-1A previously approved these changes Nov 15, 2024
/** Determine if the reviewers are bypassed */
/**
* Determine if the reviewers are bypassed
* Note: This option is not supported on Github anymore due to Github Api change.
Copy link
Contributor

@matthieu-crouzet matthieu-crouzet Nov 18, 2024

Choose a reason for hiding this comment

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

Why are not we deprecating it ?
If we want to be able to re-use that part of the code for another platform,
we may extract that part in a dedicated package
else it seems to be stricly done for GitHub and then we should deprecate this proprety

Copy link
Contributor Author

@kpanot kpanot Nov 18, 2024

Choose a reason for hiding this comment

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

I did not deprecate this option because I think that as the cascading is already today platform agnostic (event if it is implemented only for Github) and this option still make sense for supporting platform, it would be weird to remove it in 1 year to follow the unique implementation to potentially seeing it reappear after a while because of a new implementation.
It made more sense for me to keep it and warn the Github users instead, but I am OK to change if needed.

PS: To implement another platform support, there is no need to extract code, it's the bot that should switch the implementation according to the received message. So today it is already ready to support multi platform as it is

@@ -2,6 +2,7 @@
"$schema": "../apps/github-cascading-app/schemas/config.schema.json",
"bypassReviewers": true,
"labels": ["cascading"],
"onlyCascadeOnHighestMinors": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably not activate it for us

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why ? I was thinking that it is actually the issue we have today with the non-removed prerelease branches and the policy supporting only the latest minor of each major?

@cpaulve-1A cpaulve-1A self-requested a review November 18, 2024 06:56
@cpaulve-1A cpaulve-1A dismissed their stale review November 18, 2024 06:59

Saw a configuration I would discuss of

Copy link
Member

@vscaiceanu-1a vscaiceanu-1a left a comment

Choose a reason for hiding this comment

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

Is there a case where we want to skip non-latest minors?
Wouldn't this create incoherence and confusion among the different branches / code levels?

.github/.cascadingrc.json Outdated Show resolved Hide resolved
@kpanot kpanot force-pushed the feature/cascading-last-minor branch 2 times, most recently from e5651ac to 5657885 Compare November 18, 2024 12:55
@kpanot kpanot force-pushed the feature/cascading-last-minor branch from 5657885 to c4a10be Compare November 18, 2024 12:58
@kpanot
Copy link
Contributor Author

kpanot commented Nov 18, 2024

Is there a case where we want to skip non-latest minors? Wouldn't this create incoherence and confusion among the different branches / code levels?

Not sure to understand why.
As for the policy, we considered the latest minor branches as supported/maintained branches. Which mean that all the other branches are considered as archived (if they are not destroyed).
When we cascade, we want to do it only on the considered branched automatically, why would it create "incoherence" to skip unmaintained branches?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants