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

Feature/npm publish #8

Merged

Conversation

sauravhiremath
Copy link
Contributor

  • Updated build.yml to support node

Copy link
Member

@rakshith-ravi rakshith-ravi left a comment

Choose a reason for hiding this comment

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

We don't really need a matrix OS. That was for rust. For node, we can build it all on one OS.

Feel free to rewrite them as long as the basic structure is maintained

.github/workflows/pr.yml Outdated Show resolved Hide resolved
.github/workflows/pr.yml Outdated Show resolved Hide resolved
.github/workflows/pr.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
@rakshith-ravi
Copy link
Member

If you're still working on this and don't want me to review it yet, make sure you mark your PR with WIP in the beginning.

@sauravhiremath
Copy link
Contributor Author

If you're still working on this and don't want me to review it yet, make sure you mark your PR with WIP in the beginning.

Yeah, I sent to PR to npm-publish branch to check if its working. It wasn't a final push. But yeah i'll add a WIP next time

@rakshith-ravi
Copy link
Member

Any update @sauravhiremath?

@sauravhiremath
Copy link
Contributor Author

Any update @sauravhiremath?

Currently reading up on GitHub actions. Should be completed by tomorrow

@rakshith-ravi
Copy link
Member

Is this ready for review?

@sauravhiremath
Copy link
Contributor Author

Other than the asset uploading part, it is pretty much done. Took some time to understand the current build configs and gh-actions in general.

Based on our commit naming conventions I think it would be better if we go with semantic-release.

Has some really cool features out-of-the-box:

  • Creates GitHub releases to mirror the versions published to npm and adds changelogs based on above conventions and adds them to the GitHub releases.
  • Notifies contributors with comments in pull requests and resolved issues that a new version has been released.

Thoughts?

@rakshith-ravi
Copy link
Member

We do follow semver, so adopting this wouldn't be an issue, but we already have a flow that's decided on the main juno repo. What advantages would this present over that?

@sauravhiremath
Copy link
Contributor Author

sauravhiremath commented Apr 30, 2020 via email

@rakshith-ravi
Copy link
Member

Yes, and neither of those are something that compelling enough for me to switch, or something that our existing flow doesn't provide already. I see no reason to switch

@sauravhiremath sauravhiremath force-pushed the feature/npm-publish branch from ebf78ee to 35151c0 Compare May 1, 2020 06:13
@rakshith-ravi rakshith-ravi requested a review from thebongy May 1, 2020 07:47
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
.github/workflows/pr.yml Show resolved Hide resolved
@sauravhiremath sauravhiremath force-pushed the feature/npm-publish branch from 6bf04c4 to f228f8a Compare May 1, 2020 10:09
@sauravhiremath sauravhiremath requested a review from thebongy May 1, 2020 10:12
@thebongy
Copy link
Contributor

thebongy commented May 1, 2020

Lgtm, we'll have to merge and test is this works properly on the other PR

@thebongy thebongy merged commit 4916154 into bytesonus:feature/npm-publish May 1, 2020
@sauravhiremath
Copy link
Contributor Author

Lgtm, we'll have to merge and test is this works properly on the other PR

I have tested it on my repo. So it shouldn't be a problem

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

Successfully merging this pull request may close these issues.

3 participants