-
Notifications
You must be signed in to change notification settings - Fork 21
chore: keep generated/main
alive
#349
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
Conversation
✅ Deploy Preview for api-clients-automation canceled.
|
✗ The generated branch has been deleted.If the PR has been merged, you can check the generated code on the |
06a5ecf
to
f348b20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of complicated to test, let's see it in action, great job !
@@ -3,6 +3,8 @@ import { run } from '../../common'; | |||
import { configureGitHubAuthor } from '../../release/common'; | |||
import { getNbGitDiff } from '../utils'; | |||
|
|||
import { GENERATED_MAIN_BRANCH } from './text'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed that GENERATED_MAIN_BRANCH
will pull a dependency from the release
folder, can you put MAIN_BRANCH
in the root common.ts
pls ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, we need re-arrange the scripts folder imo, there's a lot of duplicate or deps that does not really make sense
@@ -3,6 +3,8 @@ import { run } from '../../common'; | |||
import { configureGitHubAuthor } from '../../release/common'; | |||
import { getNbGitDiff } from '../utils'; | |||
|
|||
import { GENERATED_MAIN_BRANCH } from './text'; | |||
|
|||
const PR_NUMBER = parseInt(process.env.PR_NUMBER || '0', 10); | |||
const FOLDERS_TO_CHECK = 'yarn.lock openapitools.json clients specs/bundled'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this should be a list and getNbGitDiff
accept a list, but I should have said that in the last PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was at first but then I had to join everything so I've went with a simple string, we will definitely need to iterate on this part so it can change anytime actually
I guess we can leave those changes for when we will remove generated code from main
// For the GENERATED_MAIN_BRANCH, we take the latest commit on main and generate code | ||
if (baseBranch === 'main') { | ||
console.log(`Merging '${baseBranch}' in '${generatedCodeBranch}'`); | ||
await run(`git merge --no-commit ${baseBranch}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So generated/main
will move by 2 commits for each PR ? Is it possible to squash the merge and the changes of the PR ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't create a merge commit, so it should only be ahead of one generation commit for each PR merged to main, which keeps the history correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Squashing was my first option, but can lead to wrong authoring if multiple commits are merged to main. Which could also be solved by avoid cancelling concurrent jobs on main (2nd point in the PR body)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome !
export const MAIN_BRANCH = config.mainBranch; | ||
export const GENERATED_MAIN_BRANCH = `generated/${MAIN_BRANCH}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this make more sense 👍
0210f20
to
e26ffbc
Compare
🧭 What and Why
🎟 JIRA Ticket: https://algolia.atlassian.net/browse/APIC-389
Changes included:
This PR should enable future iterations on removing generated stuff from
main
while keepinggenerated/main
up to date.ref
when the current branch is notmain
main
ingenerated/main
before pushing the generated commitcompare
UI with thegenerated/main
branchNotes:
concurrency
option on main to avoid wrong authoring.🧪 Test
CI :D