-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 auth support #21
Add auth support #21
Conversation
@arcanis could you review this and make sure this will work for pulling and pushing packages to private registries with YARN? |
This should be good to go, I verified it works for both GPR and npmjs for Yarn and npm. Note that there are a lot of file changes because I added the github package and lots of node_modules changed as a result. That's not super important though. Files you should be looking at for this review are
|
Can we remove the need to specify |
@jclem, as mentioned elsewhere that would cause auth to get configured by default. With that said, I think it would be nice to somehow configure this by default for npm users - are you ok pushing this off til past 8/8 though? Can at the very least include an example of authenticating against npm and GPR in the README for this pr though. EDIT: Added some examples to the readme of publishing to npmjs and GPR |
src/authutil.ts
Outdated
} | ||
|
||
function writeRegistryToFile(registryUrl: string, fileLocation: string) { | ||
let scope = core.getInput('scope'); |
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.
Const + type
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.
It gets modified so it needs to be let. Added types
@jclem I'm going to merge. If we decide we need something different for npm users we can add a follow up PR |
const curContents = fs.readFileSync(fileLocation, 'utf8'); | ||
curContents.split(os.EOL).forEach((line) => { | ||
// Add current contents unless they are setting the registry | ||
if (!line.toLowerCase().startsWith('registry')) { |
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.
This doesn't seem right. Only the line(s) that is being redefined should be removed.
registry
defines the default registry for unscoped dependencies, whereas if scope is provided this function is (re)defining a scoped registry.
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.
FYI @hross
sandbox-api path change
Bumps [acorn](https://github.com/acornjs/acorn) from 7.1.0 to 7.1.1. - [Release notes](https://github.com/acornjs/acorn/releases) - [Commits](acornjs/acorn@7.1.0...7.1.1) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
No description provided.