-
Notifications
You must be signed in to change notification settings - Fork 21
chore(ci): support independent versioning #248
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. 🔨 Explore the source changes: 69a1db1 🔍 Inspect the deploy log: https://app.netlify.com/sites/api-clients-automation/deploys/6239dace48642b0009592f15 |
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.
Massive work ! I don't understand half of it, let's see it in action !
✗ The generated branch has been deleted. If the PR has been merged, you can check the generated code on the |
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.
Looks really good!!
scripts/release/common.ts
Outdated
// eslint-disable-next-line no-param-reassign | ||
mainGenerator[lang] = clientsConfig[lang].mainGenerator; | ||
return mainGenerator; |
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.
You can remove the eslint silent
// eslint-disable-next-line no-param-reassign | |
mainGenerator[lang] = clientsConfig[lang].mainGenerator; | |
return mainGenerator; | |
return { | |
...mainGenerator, | |
[lang]: clientsConfig[lang].mainGenerator, | |
}; |
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.
In this case, I prefer reassign. By its nature, reduce
involves reassigning. eslint error about this within the context of reduce seems dumb to me. Plus, just to avoid eslint error, spreading the object can be cost-y (of course, in our case, it's almost zero, though)
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.
eslint error about this within the context of reduce seems dumb to me.
I agree, it's also because it makes it more readable that way (IMO), but not a blocker anyway!
Co-authored-by: Clément Vannicatte <[email protected]>
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.
LGTM
Thought that is coming late to the party.
I'm surprised to see that much custom code, I was in the impression that we could reuse ShipJS (probably too JS specific but some parts) or semantic-release
that is mostly language agnostic.
For example the commit parsing is tricky and has already been done. Same for changelog processing.
Maybe I'm missing some stuff
Ship.js is indeed too JS specific. We're dealing with multiple languages at the same time, while supporting independent versioning (which didn't exist in Ship.js). I copied & pasted code from Ship.js here and there. For changelog processing, I've used |
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.
Let's test it!
We should iterate on the Language
type thing before we forget but it's not urgent here
yup, we've got it here https://algolia.atlassian.net/browse/APIC-385 |
return versions; | ||
function readVersions(): VersionsWithoutReleaseType { | ||
return Object.keys(MAIN_GENERATOR).reduce((acc, lang) => { | ||
// eslint-disable-next-line no-param-reassign |
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.
There's this one also 😇
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.
what a hawk eye. 69a1db1
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.
👀 🐦
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.
purrfect
* chore: replace next with releaseType * chore: parse new issue body correctly * chore(ci): support independent versioning * chore: fix version bumping * chore: fix test cases * chore: add more test * chore: move mainGenerator to clients.config.json * chore: clean up code * chore: rename variables * chore: extract types to types.ts * Update scripts/release/__tests__/process-release.test.ts Co-authored-by: Clément Vannicatte <[email protected]> * chore: update types * chore: update test * chore: fix broken config * chore: avoid eslint error * chore: avoid eslint error Co-authored-by: Clément Vannicatte <[email protected]>
🧭 What and Why
🎟 JIRA Ticket: https://algolia.atlassian.net/browse/APIC-364
Changes included:
🧪 Test