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

feat!: template pulling with npm config #1069

Merged

Conversation

pierrick-boule
Copy link
Contributor

@pierrick-boule pierrick-boule commented Nov 17, 2023

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

Copy link
Contributor

@github-actions github-actions bot left a 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.

@pierrick-boule pierrick-boule force-pushed the asyncapi_generator_npm_config branch from 5fb8462 to 7a78759 Compare November 17, 2023 23:02
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@pierrick-boule pierrick-boule changed the title Asyncapi generator template pulling with npm config feat: Asyncapi generator template pulling with npm config Nov 17, 2023
@pierrick-boule pierrick-boule changed the title feat: Asyncapi generator template pulling with npm config feat: asyncapi generator template pulling with npm config Nov 17, 2023
@pierrick-boule
Copy link
Contributor Author

Hello,
The indent was not to make a dupplicate of #1061 , I was scripting the generator at work this week and been trying to work around the limits of the generator (our ci has no direct acces to the web and to ways : private repository, http proxy with auth). Both are ok with npm because we can config it but not with the generator.
Maybe this PR can help to find a convenient way of using the generator as easily that npm by reusing the same source of configuration.

@pierrick-boule pierrick-boule changed the title feat: asyncapi generator template pulling with npm config feat: template pulling with npm config Nov 18, 2023
Copy link
Member

@derberg derberg left a 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

@aeworxet
Copy link

@asyncapi/bounty_team

@derberg
Copy link
Member

derberg commented Jan 18, 2024

@pierrick-boule as I wrote before. This #1061 is already merged, so please update your PR with latest private npm support

@pierrick-boule pierrick-boule force-pushed the asyncapi_generator_npm_config branch 2 times, most recently from e9f8a32 to 7b78b75 Compare January 25, 2024 14:54
@pierrick-boule pierrick-boule force-pushed the asyncapi_generator_npm_config branch from 7b78b75 to 35eb66c Compare February 6, 2024 09:51
@pierrick-boule
Copy link
Contributor Author

Hello @derberg, i updated this PR, was able to test localy :
I put "127.0.0.1 registry.npmjs.org" in /etc/hosts to make it unavailable (to remove later)
set a private registry :
npm config set registry https://private.registry.org/
Run few test OK
used a docker proxy https://hub.docker.com/r/thelebster/docker-squid-simple-proxy/
npm config set proxy http://username:password@localhost:3128
Run few test OK

@pierrick-boule
Copy link
Contributor Author

Hello, i fixed the code style.
The backwards compatibility with nodeJS 12 is not that easy to get, i will need more time to check the old npm code :-( .

@pierrick-boule pierrick-boule force-pushed the asyncapi_generator_npm_config branch from 1c0d8a6 to 69925f3 Compare February 14, 2024 17:17
Copy link
Member

@derberg derberg left a 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:

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

lib/generator.js Show resolved Hide resolved
@pierrick-boule
Copy link
Contributor Author

Hello,
Yes, I tried few implementation of npm config of npm 12 and it's very different of this PR, it will be verbose to implement a retrocompatible version and not very clear/maintainable, i was also thinking about depreciating it, and maybe introduce the new LTS of node (20) if possible.

I will now focus on this way (taking your proposals).

@derberg
Copy link
Member

derberg commented Mar 19, 2024

@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:

  • please update title to feat!
  • lets refresh node support so make sure package.json and docs talk about Node 18 as minimum
  • make sure testing is also done Node 18 and 20
  • make sure package-lock is using lockfileVersion set to 3

@pierrick-boule pierrick-boule force-pushed the asyncapi_generator_npm_config branch from 2c1e7e3 to cf28e56 Compare March 19, 2024 12:13
@pierrick-boule pierrick-boule changed the title feat: template pulling with npm config feat! template pulling with npm config Mar 19, 2024
@pierrick-boule pierrick-boule changed the title feat! template pulling with npm config feat!: template pulling with npm config Mar 19, 2024
@derberg
Copy link
Member

derberg commented Mar 25, 2024

@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 package-lock.json

@pierrick-boule pierrick-boule force-pushed the asyncapi_generator_npm_config branch from cf28e56 to 8e9c82d Compare March 26, 2024 08:00
@pierrick-boule pierrick-boule requested a review from derberg March 26, 2024 08:01
@pierrick-boule pierrick-boule force-pushed the asyncapi_generator_npm_config branch from 8e9c82d to 6c4337f Compare April 8, 2024 09:56
@pierrick-boule
Copy link
Contributor Author

Hello, @derberg it should be ok now.

@pierrick-boule pierrick-boule force-pushed the asyncapi_generator_npm_config branch from 6c4337f to 35e33d2 Compare April 16, 2024 07:43
Copy link
Member

@derberg derberg left a 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.

add below file, config.js
Screenshot 2024-04-16 at 17 28 21

with content like

const npmConfig = jest.genMockFromModule('@npmcli/config');

module.exports = npmConfig;

@pierrick-boule pierrick-boule force-pushed the asyncapi_generator_npm_config branch from 35e33d2 to ade2aab Compare April 22, 2024 07:22
@pierrick-boule pierrick-boule requested a review from derberg April 22, 2024 07:25
Copy link
Member

@derberg derberg left a 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

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Member

@derberg derberg left a 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

@derberg
Copy link
Member

derberg commented Apr 24, 2024

/rtm

@asyncapi-bot asyncapi-bot merged commit 63f8b58 into asyncapi:master Apr 24, 2024
15 checks passed
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@derberg
Copy link
Member

derberg commented Apr 24, 2024

@all-contributors please add @pierrick-boule for code,test,doc

Copy link
Contributor

@derberg

I've put up a pull request to add @pierrick-boule! 🎉

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

Successfully merging this pull request may close these issues.

5 participants