feat(node-project): Added ability to provide scoped private packages #1688

AminFazlMondo · 2022-03-15T03:21:33Z

Currently there is no way for the packages that have some dependencies that are private to use projen. Thia allows scoped packages (eg, @myscope) to have different registryUrl and have code artifact login.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-commenter · 2022-03-15T03:33:51Z

Codecov Report

Merging #1688 (63c600f) into main (d90284c) will increase coverage by 1.88%.
The diff coverage is 91.53%.

@@            Coverage Diff             @@
##             main    #1688      +/-   ##
==========================================
+ Coverage   88.06%   89.94%   +1.88%     
==========================================
  Files         132      153      +21     
  Lines        5109     6233    +1124     
  Branches     1207     1595     +388     
==========================================
+ Hits         4499     5606    +1107     
- Misses        610      622      +12     
- Partials        0        5       +5

Impacted Files	Coverage Δ
src/cdk/consts.ts	`100.00% <ø> (+36.36%)`	⬆️
src/dev-env.ts	`83.33% <0.00%> (ø)`
src/java/index.ts	`100.00% <ø> (ø)`
src/python/index.ts	`100.00% <ø> (ø)`
src/release/bump-version.ts	`98.68% <ø> (+0.05%)`	⬆️
src/release/index.ts	`100.00% <ø> (ø)`
src/release/publisher.ts	`98.85% <ø> (-0.38%)`	⬇️
src/release/release-trigger.ts	`100.00% <ø> (ø)`
src/release/release.ts	`95.79% <ø> (-2.06%)`	⬇️
src/release/tag-version.ts	`86.66% <ø> (ø)`
... and 184 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd2f1e0...63c600f. Read the comment docs.

Chriscbr

Cool! I've added a few small comments. Could you please add a section in our hand-written docs to document this feature? Just a few sentences would be great. :) Perhaps in docs/node.md, or wherever you see most appropriate.

(This is just brainstorming, nothing that needs to be addressed): How does this experience work locally / how do you authenticate to install packages when projen isn't running in CICD? I wonder if we should also add some kind of npx projen codeartifact:auth command, or some kind of "install hook" that allows you to specify one or more commands to run before installing dependencies in any task. Or is there a simpler way, like generating a .npmrc file, etc...

src/javascript/node-project.ts

test/node-project.test.ts

AminFazlMondo · 2022-03-16T09:40:29Z

Cool! I've added a few small comments. Could you please add a section in our hand-written docs to document this feature? Just a few sentences would be great. :) Perhaps in docs/node.md, or wherever you see most appropriate.

(This is just brainstorming, nothing that needs to be addressed): How does this experience work locally / how do you authenticate to install packages when projen isn't running in CICD? I wonder if we should also add some kind of npx projen codeartifact:auth command, or some kind of "install hook" that allows you to specify one or more commands to run before installing dependencies in any task. Or is there a simpler way, like generating a .npmrc file, etc...

I was thinking about that too, but wanted to iterate on it with others' input. Will investigate that in future.

hoegertn · 2022-03-17T13:23:55Z

Support for scoped registries is already there using the NpmRc class. Would it be possible to read this to generate the login in CICD instead of adding another way of configuring this?

Chriscbr · 2022-03-17T20:33:13Z

@AminFazlMondo what do you think?

AminFazlMondo · 2022-03-17T21:19:44Z

That is right, we probably could use the npmRc for the registry but the code artifact token is required for the private one, soe It is right to say that this is more to generate token in CICD pipeline.
Are we suggestion that some of the code can be changed?

charliehart-mondo · 2022-03-22T06:38:53Z

Support for scoped registries is already there using the NpmRc class. Would it be possible to read this to generate the login in CICD instead of adding another way of configuring this?

I think the local npmrc file option is ok if that's the correct route to go, but it still needs to have the token setup presumably as an environment variable in a workflow step.

This is from the docs: https://docs.npmjs.com/using-private-packages-in-a-ci-cd-workflow#create-and-check-in-a-project-specific-npmrc-file

The codeartifact-auth step in this PR currently does "npm config set" for the relevant details (registry, _authToken, always_auth), it (or a different action) could be changed to update an environment variable instead and combine that with an npmrc file (would this be auto generated/updated based on config?)

https://github.com/MondoPower/codeartifact-auth/blob/main/src/index.ts

Either way, I suspect most of the code in this PR is required to add a login step regardless of if it does "npm config set" or sets an env variable, so which approach seems to fit best?

docs/node.md

src/javascript/node-project.ts

test/node-project.test.ts

src/javascript/node-package.ts

Chriscbr · 2022-03-22T22:04:57Z

src/javascript/node-package.ts

+      scopedPackagesOptions: this.parseScopedPackagesOptions(
+        options.scopedPackagesOptions
+      ),


Can we pass in options.codeArtifactOptions so that the user doesn't have to specify the options in both scopedPackagesOptions and codeArtifactOptions ?

Chriscbr · 2022-03-22T22:10:18Z

Managing the config via npmrc (with the NpmConfig component) could make sense, but it might be awkward since projen can't completely manage the file if it's being updated whenever someone runs aws codeartifact login or runs npm config set .... So I'm ok with this approach

AminFazlMondo · 2022-03-29T12:26:17Z

Hi @Chriscbr ,

Thanks for you feedback, after that it made us re-think the solution.
Originally, we wanted to support for the situation in which the code artifact details to publish to be different from the ones specified in the scoped packages. With what you mentioned above, we came to the conclusion that it is not a common use case and used the same details for scoped packages for simplicity.
We also made a task (ca:login) to login to code artifact (thanks for your suggestion), which can also be used in local development.
We would appreciate you having another look and share your thoughts with us or if looks good to you approve this.
Cheers,

Co-authored-by: Christopher Rybicki <crybicki98@gmail.com>

Co-authored-by: Charlie Hart <charlie.hart@mondo.com.au> Co-authored-by: Kevin <MondoKevin@users.noreply.github.com>

Chriscbr

This looks really great @AminFazlMondo ! I've left just a few more comments, but I think we're close to getting this merged in. 🙂

Chriscbr · 2022-04-06T00:02:08Z

docs/node.md

+
+All npm packages have a name. Some package names also have a scope. A scope follows the usual rules for package names (URL-safe characters, no leading dots or underscores). When used in package names, scopes are preceded by an @ symbol and followed by a slash, e.g. `@somescope/somepackagename`
+
+This feature supports specifying options on how to access packages in each of the scopes, otherwise will try to install from default npm registry.


Suggested change

This feature supports specifying options on how to access packages in each of the scopes, otherwise will try to install from default npm registry.

This feature supports specifying options on how package managers should access packages in each of the scopes. If no options are specified, npm or yarn will try to install scoped packages from thepublic npm registry.

src/javascript/node-package.ts

docs/node.md

src/javascript/node-project.ts

Chriscbr · 2022-04-06T00:12:17Z

src/javascript/node-project.ts

+            "aws-secret-access-key": secretToString(
+              parsedCodeArtifactOptions.secretAccessKeySecret
+            ),
+            "aws-region": "us-east-2",


Is there a reason this is hard-coded? Can we make it customizable?

Good pick up.

It's possible that a user will have multiple scopes each in a different region (and in fact account if they have access), so we need to ensure those details are passed to the ca:login script so it can generate an auth token in the correct region/account (via the --region and --domain-owner cli options) - I'll update that shortly.

However, the region you set for configure-aws-credentials can be independent of those if I'm not mistaken so long as you override it in the cli (it is a required property for that github action unfortunately ), We could grab the first repositoryurl from the array of scoped packages, parse the url and get the region from there - or we could leave it hard-coded.

I'm going to suggest we leave it hard-coded

We could obviously also add another property to set it in the options, I'm not sure if it's best/helps though?

Chriscbr · 2022-04-06T00:15:21Z

src/javascript/node-package.ts

@@ -382,6 +407,13 @@ export class NodePackage extends Component {
   */
  readonly codeArtifactOptions?: CodeArtifactOptions;


If I understand correctly, this field is being used to also provide access to Code Artifact for installing packages (not just publishing). Please update the description to reflect this

src/javascript/node-package.ts

Chriscbr · 2022-04-06T00:22:44Z

docs/node.md

+
+## Features
+
+### Scoped Private Packages


Please update this section to also explain that for the feature to work on GitHub, the user must add secrets named AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY to their repo (I believe this is true?)

The user can set custom secret names so long as that is passed to the codeartifact options. From there configure-aws-credentials will use the default or custom secret names that get passed along. I'll update the docs to reflect that codeartifact options are required (will that be enough to prompt users to set those secrets given it's a requirement for that?)

Co-authored-by: Christopher Rybicki <crybicki98@gmail.com>

…fix up docs

Chriscbr

Looks great! Thanks @AminFazlMondo and @charliehart-mondo :)

Chriscbr suggested changes Mar 15, 2022

View reviewed changes

src/javascript/node-project.ts Outdated Show resolved Hide resolved

test/node-project.test.ts Outdated Show resolved Hide resolved

AminFazlMondo requested a review from Chriscbr March 17, 2022 00:15

Chriscbr suggested changes Mar 22, 2022

View reviewed changes

AminFazlMondo force-pushed the feature/ScopedPackagesAuth branch from 8ebb557 to 2524cef Compare March 29, 2022 03:04

AminFazlMondo requested a review from Chriscbr March 29, 2022 12:26

AminFazlMondo and others added 17 commits March 31, 2022 08:12

feat(node-project): Added ability to provide private packages

c6c78e1

Added tests

499149b

Updated documentation

4799153

Updated the API docs

b55fb65

Updated inventory test snapshot

bdb25f7

moved some functions to util

9ed7801

Updated the test to use shared variable

5bea522

Updated the type

9af0b45

Fixed test

e45d241

Updataed API docs

da56c75

Updated the manual docs

b40e870

Update docs/node.md

f2ca082

Co-authored-by: Christopher Rybicki <crybicki98@gmail.com>

Update docs/node.md

c870439

Co-authored-by: Christopher Rybicki <crybicki98@gmail.com>

Update src/javascript/node-package.ts

d26e668

Co-authored-by: Christopher Rybicki <crybicki98@gmail.com>

made getScopedPackageSteps a private function

9cc4091

removed redundant inputs to create project in tests

6683c16

Updated the docs

bd27269

AminFazlMondo and others added 4 commits March 31, 2022 08:17

Changed the scope packages to use code artifact options

29cefc2

Co-authored-by: Charlie Hart <charlie.hart@mondo.com.au> Co-authored-by: Kevin <MondoKevin@users.noreply.github.com>

Added check for AWS cli installed

e437f12

updated inventory snapshot

92a07db

Updated the docs

d04ad7f

AminFazlMondo force-pushed the feature/ScopedPackagesAuth branch from 979b93e to d04ad7f Compare March 30, 2022 21:18

Chriscbr suggested changes Apr 6, 2022

View reviewed changes

charliehart-mondo and others added 2 commits April 12, 2022 14:42

Apply suggestions from code review

db6410b

Co-authored-by: Christopher Rybicki <crybicki98@gmail.com>

use region and domain owner from the scoped packages registryUrl and …

2186306

…fix up docs

Chriscbr approved these changes Apr 12, 2022

View reviewed changes

Merge branch 'main' into feature/ScopedPackagesAuth

63c600f

mergify bot merged commit bde910c into projen:main Apr 12, 2022

ahammond mentioned this pull request May 10, 2022

better handling of scoped packages (@time-loop) time-loop/clickup-projen#41

Closed

AminFazlMondo deleted the feature/ScopedPackagesAuth branch May 20, 2022 05:11

AminFazlMondo mentioned this pull request Feb 7, 2023

fix(node): CodeArtifact auth with GitHub OIDC is missing workflow permissions #2399

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(node-project): Added ability to provide scoped private packages #1688

feat(node-project): Added ability to provide scoped private packages #1688

AminFazlMondo commented Mar 15, 2022 •

edited by Chriscbr

Loading

codecov-commenter commented Mar 15, 2022 •

edited

Loading

Chriscbr left a comment •

edited

Loading

AminFazlMondo commented Mar 16, 2022

hoegertn commented Mar 17, 2022

Chriscbr commented Mar 17, 2022

AminFazlMondo commented Mar 17, 2022

charliehart-mondo commented Mar 22, 2022

Chriscbr Mar 22, 2022

Chriscbr commented Mar 22, 2022

AminFazlMondo commented Mar 29, 2022

Chriscbr left a comment

Chriscbr Apr 6, 2022

Chriscbr Apr 6, 2022

charliehart-mondo Apr 12, 2022

charliehart-mondo Apr 12, 2022

Chriscbr Apr 6, 2022

Chriscbr Apr 6, 2022

charliehart-mondo Apr 12, 2022

Chriscbr left a comment


		All npm packages have a name. Some package names also have a scope. A scope follows the usual rules for package names (URL-safe characters, no leading dots or underscores). When used in package names, scopes are preceded by an @ symbol and followed by a slash, e.g. `@somescope/somepackagename`

		This feature supports specifying options on how to access packages in each of the scopes, otherwise will try to install from default npm registry.

		@@ -382,6 +407,13 @@ export class NodePackage extends Component {
		*/
		readonly codeArtifactOptions?: CodeArtifactOptions;

feat(node-project): Added ability to provide scoped private packages #1688

feat(node-project): Added ability to provide scoped private packages #1688

Conversation

AminFazlMondo commented Mar 15, 2022 • edited by Chriscbr Loading

codecov-commenter commented Mar 15, 2022 • edited Loading

Codecov Report

Chriscbr left a comment • edited Loading

Choose a reason for hiding this comment

AminFazlMondo commented Mar 16, 2022

hoegertn commented Mar 17, 2022

Chriscbr commented Mar 17, 2022

AminFazlMondo commented Mar 17, 2022

charliehart-mondo commented Mar 22, 2022

Choose a reason for hiding this comment

Chriscbr commented Mar 22, 2022

AminFazlMondo commented Mar 29, 2022

Chriscbr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chriscbr left a comment

Choose a reason for hiding this comment

AminFazlMondo commented Mar 15, 2022 •

edited by Chriscbr

Loading

codecov-commenter commented Mar 15, 2022 •

edited

Loading

Chriscbr left a comment •

edited

Loading