Skip to content

Add auth support - #21

Merged
damccorm merged 30 commits into
masterfrom
auth
Aug 6, 2019
Merged

damccorm merged 30 commits into
masterfrom
auth

Conversation

@damccorm

@damccorm damccorm commented Aug 5, 2019

Copy link
Copy Markdown
Contributor

No description provided.

@chrispat

chrispat commented Aug 5, 2019

Copy link
Copy Markdown
Contributor

@arcanis could you review this and make sure this will work for pulling and pushing packages to private registries with YARN?

Comment thread src/authutil.ts Outdated
Comment thread src/authutil.ts Outdated
Comment thread src/authutil.ts Outdated
Comment thread src/authutil.ts Outdated
Comment thread src/authutil.ts Outdated
Comment thread src/authutil.ts Outdated
Comment thread src/authutil.ts Outdated
Comment thread src/authutil.ts Outdated
Comment thread src/setup-node.ts Outdated
Comment thread README.md Outdated
Comment thread src/authutil.ts Outdated
Comment thread src/authutil.ts Outdated
@damccorm

damccorm commented Aug 6, 2019 •

Copy link
Copy Markdown
Contributor Author

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

  • src/setup-node.ts
  • src/authutil.ts
  • README.md
  • __tests__/authutil.test.ts
  • __tests__/__snapshots__/authutil.test.ts.snap
  • __tests__/installer.test.ts
  • action.yml

@jclem

jclem commented Aug 6, 2019

Copy link
Copy Markdown

Can we remove the need to specify registry-url in order to get publishing to NPM working?

@damccorm

damccorm commented Aug 6, 2019 •

Copy link
Copy Markdown
Contributor Author

@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

Comment thread src/authutil.ts Outdated
}

function writeRegistryToFile(registryUrl: string, fileLocation: string) {
let scope = core.getInput('scope');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Const + type

Copy link
Copy Markdown
Contributor Author

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

@damccorm

damccorm commented Aug 6, 2019

Copy link
Copy Markdown
Contributor Author

@jclem I'm going to merge. If we decide we need something different for npm users we can add a follow up PR

@damccorm
damccorm merged commit 78148da into master Aug 6, 2019
@damccorm
damccorm deleted the auth branch September 10, 2019 17:32
Comment thread lib/authutil.js
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')) {

@joebowbeer joebowbeer Feb 27, 2020 •

Copy link
Copy Markdown

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

FYI @hross

krzyk pushed a commit to krzyk/setup-node that referenced this pull request Apr 11, 2023
deining pushed a commit to deining/setup-node that referenced this pull request Nov 9, 2023
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] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@aguilarj0987-oss

Copy link
Copy Markdown

yes>

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.

10 participants