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

[MWPW-147001] Stage initiative automation (Stage-Main sync PRs) #2224

Merged
merged 9 commits into from
May 8, 2024

Conversation

mokimo
Copy link
Contributor

@mokimo mokimo commented Apr 29, 2024

Based on the discussion, the first workflow that will merge PRs into stage and open stage->main syncs.

PRs will be merged right after applying the Ready for Stage label if possible, or in a 24h interval. Once we have the stage->main MERGE workflow, that will also trigger opening a new stage->main sync instantly.

Resolves: MWPW-147001

Test URLs:

Copy link
Contributor

aem-code-sync bot commented Apr 29, 2024

Page Scores Audits Google
/?martech=off PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@mokimo mokimo added the needs-verification PR requires E2E testing by a reviewer label Apr 29, 2024
Copy link
Contributor

@overmyheadandbody overmyheadandbody left a comment

Choose a reason for hiding this comment

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

Is my understanding correct that merge to main would still be manual for the time being? Just asking because I know we were discussing of a window where a merge to main could occur.

.github/workflows/helpers.js Show resolved Hide resolved
// Run from the root of the project for local testing: node --env-file=.env .github/workflows/merge-to-stage.js
const prTitle = '[Release] Stage to Main';
const seen = {};
const requiredApprovals = process.env.REQUIRED_APPROVALS || 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this take code owners into account? Say that a PR brings changes to the marquee and 2 reviewers approve, none of which are listed as code owners for the marquee. Should the approval "count" in that scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't require this at the moment either - 2 approvals & that counts. Code owners are just automatically looped in/requested for reviews

Copy link
Contributor

@3ch023 3ch023 May 2, 2024

Choose a reason for hiding this comment

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

true, but it's a good point from @overmyheadandbody , maybe in the future we will need this..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Github offers this OOTB for a whole repo, we can still decide to enable this at some point
Screenshot 2024-05-02 at 16 38 28

So far it hasn't been an issue and hope it won't be in the future either - but the option is there

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, maybe someone de-activated it recently? A couple of months ago at least one code owner had to review the PR, even though it had more than 2 approvals already. And I'd say it's fair enough, that's one of the reasons we have the codeowners in the first place - automatically assign the specialized person and have them give their two cents. This might deserve some further discussion to understand who and why disabled this rule.

.github/workflows/merge-to-stage.js Show resolved Hide resolved
.github/workflows/merge-to-stage.js Show resolved Hide resolved
.github/workflows/merge-to-stage.js Show resolved Hide resolved
Copy link
Contributor

@3ch023 3ch023 left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

github-actions bot commented May 3, 2024

This PR is currently in the needs-verification state. Please assign a QA engineer to verify the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready for Stage verified PR has been E2E tested by a reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants