Skip to content

Conversation

@fboucquez
Copy link
Contributor

@fboucquez fboucquez commented Feb 25, 2020

Addresses issue #458. Please read the reason for this PR there.

Once this PR is merged, Travis will start deploying an alpha version to npm every master build (nodejs version 8 builder).

The notation is

symbol-sdk:$version-alpha-$datetime

For example

symbol-sdk:0.17.2-alpha-202002251427 (tag alpha)

Note that the alpha version uses the next release version, right now it would be 0.17.2-alpha-xxxx. Once 0.17.2 is released, the next alpha version would be 0.17.3-alpha-xxxx

I couldn't find any --pre option in
https://docs.npmjs.com/cli/publish

Also, the --tag doesn't allow redeploy if the version is the same. The travis script generates a new alpha version when executed

Note: NPM_TOKEN needs to be provided in https://travis-ci.org/nemtech/symbol-sdk-typescript-javascript/settings. I could use my token but my account would be the one publishing. Ideally, we should have a bot npm user.

The alpha version and tag name can be changed for anything. Maybe prerelease?

@rg911 rg911 requested a review from evias February 26, 2020 08:32
Copy link
Contributor

@rg911 rg911 left a comment

Choose a reason for hiding this comment

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

Not sure we need this pre / alpha release regarding continuous delivery. If we automatically release on merging on the master branch (assuming all tested), I would consider just using the actual incremental versions.

@fboucquez
Copy link
Contributor Author

fboucquez commented Feb 26, 2020

In a perfect world, I would go with patch releases every master merge. I see some problems now:

  1. SDK releases require some manual steps like writing down the Change Log, create GitHub releases from tags with the changelog, notify users via slack. Git tagging and npm version increase. We need to automatize everything (which it's possible but not that simple) before allowing master releases. If not you would need to manually add a new entry into the changelog every new PR. Keeping up with the versioning when it's not automatic it's hard. You don't know the order of when a PR will be merged to master and released.
  2. We may not want to publically release a new version if a dependency hasn't been publically released yet (like the current 0.9.3.1 compatible private releases)
  3. We may not want to increase a patch version every small PR merge like a doc fix, unit test added or some small code clean up.

In Java, we are using the special release branch to perform the Travis patch releases where git gets tagged, the version gets upgraded, release artifacts are uploaded, etc. We are still doing the manual steps of writing the changelog, creating the GitHub release from a tag and the Slack notification.

In Java, a master merge means snapshot deployment, a release merge means release deployment

@fboucquez fboucquez requested a review from rg911 February 26, 2020 12:14
@fboucquez
Copy link
Contributor Author

fboucquez commented Mar 5, 2020

@rg911 I've tested the dependency resolution in npm

The user needs to explicitly tell npm when installing that it wants a particular tag or alpha version

https://medium.com/@kevinkreuzer/publishing-a-beta-or-alpha-version-to-npm-46035b630dd7

Auto-upgrade only works on the latest released version (x.y.z), without any suffix.

So:

If you have something like in your package.json

"symbol-openapi-typescript-node-client": "^0.8.2",

It will bring the artifact number 0.8.5 although there are newer alpha/unreleased ones (0.8.6-xxxx)

https://www.npmjs.com/package/symbol-openapi-typescript-node-client

When we merge this, the user would need to explicitly tell npm that they one a particular alpha version. I don't think an unreleased artifact will seek into anybody that wants a release version

Try

npm install symbol-openapi-typescript-node-client -save

only 0.8.5 will be installed

Improved exception handling when the Observable error is in the code, not the http response.
Comment on lines +82 to 86
if (error instanceof Error) {
return error;
}
return new Error(error);
}
Copy link
Contributor Author

@fboucquez fboucquez Mar 6, 2020

Choose a reason for hiding this comment

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

Comment about his change: If we just wrap the exception into a new one Error, the original stack trace is lost. We want the stack trace when something in our code fails, like an NPE when mapping the DTO.

@rg911 rg911 merged commit e3693e5 into symbol:master Mar 6, 2020
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.

2 participants