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.

Sign up for GitHub

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

Jump to bottom

feat: create GitHub releases #311

Merged
merged 14 commits into from
Oct 13, 2019
Merged

feat: create GitHub releases #311

merged 14 commits into from
Oct 13, 2019

Conversation

uetchy
Copy link
Contributor

@uetchy uetchy commented Oct 8, 2019

This PR injects createGitHubRelease into release.

What createGitHubRelease do

  • Get changelog from conventional-changelog
  • hub release create with changelog

Possibly fixes #109

Edit PR freely

@uetchy
Copy link
Contributor Author

uetchy commented Oct 8, 2019

I've tested on this release.

@eunjae-lee
Copy link
Contributor

@uetchy
Hi there,
Thank you so much for your contribution.
This is indeed what I was planning to do.
I see some parts that needs to be modified.
Do you want to keep working on this?
Or do you want me to take it over?

If you want to keep doing it, I will leave some comments here.
Let me know.
Thank you 😁

@uetchy
Copy link
Contributor Author

uetchy commented Oct 9, 2019

@eunjae-lee I'd like to keep working on this🚀

@eunjae-lee
Copy link
Contributor

Thanks again @uetchy for your contribution :)
Let me know what you think about my comments. If you have another opinion, please don't hesitate to tell me. Let's develop this better together 💪🏼

@uetchy
Copy link
Contributor Author

uetchy commented Oct 10, 2019 •
edited
Loading

@eunjae-lee I think shipjs is definitely going to be a crucial part of the distribution ecosystem!
So which part of this PR do you think needs to be fixed?

Have you put a PR review already? I couldn't see any comment right now.

@eunjae-lee
Copy link
Contributor

@uetchy ahhh I wrote them but they weren't submitted. Can you see them now?

@uetchy uetchy force-pushed the features/github-releases branch 2 times, most recently from b20e40b to 5f67ad8 Compare October 10, 2019 12:59
@uetchy
Copy link
Contributor Author

uetchy commented Oct 10, 2019

@eunjae-lee All done! Could you check out the revised code?

eunjae-lee
eunjae-lee reviewed Oct 10, 2019
View reviewed changes
packages/shipjs/src/step/release/createGitHubRelease.js Outdated
// fetch CHANGELOG
exportedPath = tempWrite.sync(existingChangelog);
} else {
// generate CHANGELOG
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of not generating here at all?
For example, at shipjs prepare, conventional-changelog already generated some changelog for the current version and user didn't like it, so they removed it. And if we generate and use it again for the github release, there's nothing user can do about it.
It might make sense just to keep using what user decided at shipjs prepare step.
What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially thought it was good to generate CHANGELOG automatically for the release but then realized that it was supposed to be already generated before trigger. So you are right.
Let this section just extract matching release from CHANGELOG then generate a release on GitHub repository.

Copy link
Contributor Author

@uetchy uetchy Oct 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

How about just create a release with a blank message if there are no CHANGELOG?
If we want to add support for uploading artifacts (zip files, etc) to a release in the future, we then need to create a release even without a changelog.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about just create a release with a blank message if there are no CHANGELOG?

It totally makes sense 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eunjae-lee 👌👌
I already updated my code to do so

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! I'll try out this PR this weekend, and hopefully merge it 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eunjae-lee Yay! Thank you for taking your weekend time ☺️

@eunjae-lee
Copy link
Contributor

@uetchy While I was testing this, I've found some stuff, so added commits.
Let me know how you think.

@uetchy
Copy link
Contributor Author

uetchy commented Oct 13, 2019 •
edited
Loading

@eunjae-lee Totally reasonable fixes! wonder why I missed ‘fs.resolve’😑

@eunjae-lee eunjae-lee changed the title Ability to create GitHub Release feat: create GitHub releases Oct 13, 2019
@eunjae-lee eunjae-lee merged commit f1e4e77 into algolia:master Oct 13, 2019
@eunjae-lee
Copy link
Contributor

That's what the review is for 😁

Congrats for the first code PR at Ship.js merged! 🎉

@uetchy uetchy deleted the features/github-releases branch October 14, 2019 04:38
@uetchy
Copy link
Contributor Author

uetchy commented Oct 14, 2019

🎉🛳🍾

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eunjae-lee eunjae-lee eunjae-lee left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

why is the content not formatted on "releases" tab on github?
2 participants
@uetchy @eunjae-lee