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

Add scope/namespace option to CodeArtifact login #5404

Merged
merged 1 commit into from
Aug 11, 2020
Merged

Add scope/namespace option to CodeArtifact login #5404

merged 1 commit into from
Aug 11, 2020

Conversation

ryansonshine
Copy link
Contributor

Resolves #5291

Description of changes:

This change adds the ability to pass in a scope/namespace to CodeArtifact login.
The primary arg is namespace to remain generalized across different CodeArtifact
tools (ie: npm scope, maven groupid).

This change also includes:

  • Throw error if namespace or scope are used for configuring pip
  • Throw error if namespace or scope are used for configuring twine
  • Throw error if namespace and scope are used at the same time
  • Accept npm namespace value with or without "@" prefix for scope
  • Add "@" prefix to npm scope if user has not passed it
  • Validation on scope name to adhere with npm rules

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov-commenter
Copy link

codecov-commenter commented Jul 24, 2020

Codecov Report

Merging #5404 into develop will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #5404   +/-   ##
========================================
  Coverage    94.06%   94.07%           
========================================
  Files          190      190           
  Lines        14758    14780   +22     
========================================
+ Hits         13882    13904   +22     
  Misses         876      876           
Impacted Files Coverage Δ
awscli/customizations/codeartifact/login.py 100.00% <100.00%> (ø)

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 57e6103...dcee44d. Read the comment docs.

@ryansonshine ryansonshine marked this pull request as ready for review July 25, 2020 17:40
Copy link

@danielgmyers danielgmyers left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me.

Copy link
Contributor

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

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

Looks good! Just had a couple of suggestions.

awscli/customizations/codeartifact/login.py Outdated Show resolved Hide resolved
Copy link
Contributor

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

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

Looks great! Two more things that I forgot to mention in the last review before merging the PR:

  1. Could you squash all of your commits into a single commit?
  2. Could you add a changelog entry for this update? To do this, from the root directory of the repository, you can run the following commands to make an entry:
$ ./scripts/new-change -t enhancement -c '``codeartifact login``' -d 'Add support for ``--namespace`` parameter #5291' -r aws/aws-cli
$ git add .changes/next-release/
$ git commit

I can also make these changes and push to your branch as well and then merge the PR if you do not have time to get to it or want me to handle it. Let me know!

@KapitalEarth
Copy link

KapitalEarth commented Aug 11, 2020 via email

This change adds the ability to pass in a namespace to CodeArtifact login.
The primary arg is `namespace` to remain generalized across different
CodeArtifact tools (ie: npm scope, maven groupid).

This change also includes:

- Throw error if namespace is used for configuring pip
- Throw error if namespace is used for configuring twine
- Accept npm namespace value with or without "@" prefix for scope
- Validation on npm scope name to adhere with npm rules
@ryansonshine
Copy link
Contributor Author

Looks great! Two more things that I forgot to mention in the last review before merging the PR:

  1. Could you squash all of your commits into a single commit?
  2. Could you add a changelog entry for this update? To do this, from the root directory of the repository, you can run the following commands to make an entry:
$ ./scripts/new-change -t enhancement -c '``codeartifact login``' -d 'Add support for ``--namespace`` parameter #5291' -r aws/aws-cli
$ git add .changes/next-release/
$ git commit

I can also make these changes and push to your branch as well and then merge the PR if you do not have time to get to it or want me to handle it. Let me know!

Thanks @kyleknap . I've squashed my commits and added a changelog entry. Please let me know if you'd like me to make any additional changes.

Copy link
Contributor

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

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

Looks good! 🚢 Merging.

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.

Add scope option for npm login on CodeArtifact
5 participants