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 auth support #21

Merged
merged 30 commits into from Aug 6, 2019
Merged

Add auth support #21

merged 30 commits into from Aug 6, 2019

Conversation

@damccorm
Copy link
Contributor

@damccorm damccorm commented Aug 5, 2019

No description provided.

damccorm added 8 commits Aug 5, 2019
@chrispat chrispat requested a review from arcanis Aug 5, 2019
@chrispat
Copy link
Member

@chrispat chrispat commented Aug 5, 2019

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

src/authutil.ts Outdated Show resolved Hide resolved
src/authutil.ts Outdated Show resolved Hide resolved
src/authutil.ts Outdated Show resolved Hide resolved
src/authutil.ts Outdated Show resolved Hide resolved
src/authutil.ts Outdated Show resolved Hide resolved
src/authutil.ts Outdated Show resolved Hide resolved
src/authutil.ts Outdated Show resolved Hide resolved
src/authutil.ts Outdated Show resolved Hide resolved
src/setup-node.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/authutil.ts Outdated Show resolved Hide resolved
src/authutil.ts Outdated Show resolved Hide resolved
damccorm added 3 commits Aug 5, 2019
damccorm and others added 7 commits Aug 6, 2019
Iheanyi Ekechukwu
@iheanyi iheanyi force-pushed the auth branch from 706f892 to f20c85e Aug 6, 2019
damccorm and others added 3 commits Aug 6, 2019
iheanyi and others added 7 commits Aug 6, 2019
@damccorm
Copy link
Contributor Author

@damccorm damccorm commented Aug 6, 2019

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
Copy link
Collaborator

@jclem jclem commented Aug 6, 2019

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

@damccorm
Copy link
Contributor Author

@damccorm damccorm commented Aug 6, 2019

@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

}

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

This comment has been minimized.

This comment has been minimized.

@damccorm

damccorm Aug 6, 2019
Author Contributor

It gets modified so it needs to be let. Added types

@damccorm
Copy link
Contributor Author

@damccorm damccorm commented Aug 6, 2019

@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
2 checks passed
2 checks passed
Run (ubuntu-latest)
Details
Run (windows-latest)
Details
@damccorm damccorm deleted the auth branch Sep 10, 2019
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')) {

This comment has been minimized.

@joebowbeer

joebowbeer Feb 27, 2020

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.

This comment has been minimized.

@yanz-androidify yanz-androidify mentioned this pull request Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.