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

see if this fixes cypress issues #878

Merged
merged 2 commits into from
Feb 21, 2023
Merged

see if this fixes cypress issues #878

merged 2 commits into from
Feb 21, 2023

Conversation

ndittren
Copy link
Member

@ndittren ndittren commented Feb 21, 2023

Node 17+ changed how they resolve DNS names. Changing localhost to 127.0.0.1 is the fix for this issue

cypress-io/cypress#21430

@ndittren ndittren force-pushed the test-node branch 4 times, most recently from 7259234 to 8e8f8d1 Compare February 21, 2023 20:29
@ndittren
Copy link
Member Author

@nikolas So this gets cypress to pass but I am not sure if it's the right approach since it includes adding these hosts to the allowed list.

@nikolas
Copy link
Member

nikolas commented Feb 21, 2023

Oh interesting! Yeah, I think this would be fine to merge - I guess an even better fix would be to only add localhost to ALLOWED_HOSTS when necessary. Maybe we can do something like this:

if 'cypress' in sys.argv:
    ALLOWED_HOSTS = ['127.0.0.1', 'localhost']

@nikolas
Copy link
Member

nikolas commented Feb 21, 2023

Also, we don't need to overwrite our default ALLOWED_HOSTS from ccnmtlsettings: https://github.com/ccnmtl/ccnmtlsettings/blob/master/ccnmtlsettings/shared.py#L15

Maybe just adding 127.0.0.1 here is all we need to do. I've added a new commit.

If this looks good, we can just add this to our general ccnmtlsettings - well, or our new ctlsettings.

@nikolas nikolas merged commit d53099a into main Feb 21, 2023
@ndittren ndittren deleted the test-node branch February 21, 2023 20:57
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.

2 participants