-
-
Notifications
You must be signed in to change notification settings - Fork 239
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
feat!: template pulling with npm config #1069
feat!: template pulling with npm config #1069
Conversation
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.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
5fb8462
to
7a78759
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Hello, |
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.
@pierrick-boule sorry for keeping you waiting with review, but we were all focused on v3 release
Thanks a lot for that PR. I also do not think PR somehow contradicts the other one about private registry. Tbh the more flexible we are with generator the better for people, especially in ci/cd setups.
My proposal is that first we go first with the other PR and then adjust this one to match it, cause we will have to somehow condition which auth should be used: the one passed as options directly to library, or by reusing the existing conf
If you have a private registry you could test against the other PR, that would help out a lot
@asyncapi/bounty_team |
@pierrick-boule as I wrote before. This #1061 is already merged, so please update your PR with latest private npm support |
e9f8a32
to
7b78b75
Compare
7b78b75
to
35eb66c
Compare
Hello @derberg, i updated this PR, was able to test localy : |
Hello, i fixed the code style. |
1c0d8a6
to
69925f3
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.
@pierrick-boule there is no strong requirement to be compatible with node 12
we can definitely make this PR trigger major release because of stop of support for node12 - that is completely fine, this is a very old node version:
- just update docs
package.json
requirements- workflow: https://github.com/asyncapi/generator/blob/master/.github/workflows/pr-testing-with-test-project.yml
I'm more concerned about your custom unix-based solution with replacing paths. I'm pretty sure it will not work with windows. How about using something like https://www.npmjs.com/package/npm-run-path ?
Also, we should find a way how to test it 🤔 should not be hard as tests are already in place https://github.com/asyncapi/generator/blob/master/test/test-project/test-registry.test.js where we simulate npm environment
Hello, I will now focus on this way (taking your proposals). |
@pierrick-boule we should support what Node community supports, so minimum is Node 18 that is in maintenance until April next year -> https://nodejs.org/en/about/previous-releases since we agreed we will run a major release with this PR:
|
2c1e7e3
to
cf28e56
Compare
@pierrick-boule whenever you're ready for another review round please rerequest review as I do not get notifications that you pushed all required changes. also please solve conflicts with |
cf28e56
to
8e9c82d
Compare
8e9c82d
to
6c4337f
Compare
Hello, @derberg it should be ok now. |
6c4337f
to
35e33d2
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.
looking awesome with this new additional integration test - and CI is passing.
you just need to update https://github.com/asyncapi/generator/blob/master/docs/using-private-template.md
there is also issue with unit tests: https://github.com/asyncapi/generator/actions/runs/8701813512/job/23883476282?pr=1069
I think it is because @npm/config
is not mocked.
with content like
const npmConfig = jest.genMockFromModule('@npmcli/config');
module.exports = npmConfig;
35e33d2
to
ade2aab
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.
@Florence-Njeri please have a look at docs updates. With this PR we're releasing a new major as we will drop support for older node versions
Quality Gate passedIssues Measures |
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.
@pierrick-boule we are ready to go. I added a note about breaking change to the description. Thanks for all the hard work
/rtm |
🎉 This PR is included in version 2.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
@all-contributors please add @pierrick-boule for code,test,doc |
I've put up a pull request to add @pierrick-boule! 🎉 |
Description
Use npm config values in arborist in order to inherit form the global config when pulling a template.
This allow to pass config like private registry and proxy-auth.
Related issue(s)
#538
BREAKING CHANGE
This PR drops support for Node versions prior v18.12.0
We know this version of Generator will not work work with Node v12. We also discourage using Node v14 and v16 that are no longer under long time support from Node community. From now on, generator tests run under Node v18 and v20