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 assertion attributes to child object on profile (passport-saml#543) #5

Conversation

kriss1897
Copy link
Contributor

This attributes are also mounted to profile directly in a non conflicting way.

Duplicate PR of: node-saml/passport-saml#593

This attributes are also mounted to profile directly in a non
conflicting way.
@kriss1897
Copy link
Contributor Author

This failed build on 16.x is also failing on master. Will check and raise a PR to fix

@kriss1897
Copy link
Contributor Author

@cjbarth @zoellner The test in node v16 is failing due to a bug in npm which causes npm to break after running npm update. I checked around and found that there are two workarounds:

  1. Re-install node after running npm update
  2. Use yarn to run test script. Or can we also consider using yarn for the whole project?

@@ -24,7 +24,11 @@ jobs:
- run: npm ci
- run: npm test
- run: npm ci
- run: npm --depth 9999 update
- run: npm update
Copy link
Collaborator

Choose a reason for hiding this comment

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

How is this then working in passport-saml if it isn't working here in node-saml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried running these commands in the same sequence with passport-saml's package.json on this repo, but still the command was failing for this repo and not for passport-saml.

@cjbarth
Copy link
Collaborator

cjbarth commented May 31, 2021

@kriss1897 Do you have a link to the bug in npm?

@kriss1897
Copy link
Contributor Author

No, I didn't see any open npm tickets regarding the same. I found some old ones and some stackoverflow threads. Try this: https://www.google.com/search?q=cannot+find+module+npmlog+after+npm+update

@cjbarth
Copy link
Collaborator

cjbarth commented Jun 16, 2021

It seems that this has been fixed. Could we revert the change and try again?

@cjbarth cjbarth merged commit b98c97b into node-saml:master Jun 17, 2021
@cjbarth cjbarth added the enhancement New feature or request label Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants