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

fix: Add missing cname option not passed to the config #535

Merged
merged 3 commits into from
Dec 23, 2023

Conversation

WillBAnders
Copy link
Contributor

The --cname <CNAME> option added in #533 is not passed to the config, thus this option doesn't work in the current release. This includes that option and adds a test for the missing case.

Reviewing the original PR, the only other difference I see is that there's a nojekyll-exists spec but not a cname-exists spec - if there's a desire to have that as well I can add it.

@paymand
Copy link

paymand commented Nov 27, 2023

@WillBAnders looks like you beat me to it. I'll close my PR.

Feel free to copy over the changes I made to logging here: https://github.com/tschaub/gh-pages/pull/536/files#diff-92bbac9a308cd5fcf9db165841f2d90ce981baddcb2b1e26cfff170929af3bd1

@paymand
Copy link

paymand commented Nov 27, 2023

@WillBAnders also very good idea to add a test for cname-exists 👍

@tschaub
Copy link
Owner

tschaub commented Dec 18, 2023

Thanks for catching this and proposing a fix, @WillBAnders and @paymand. If you are able to add a regression test, that would be great. Otherwise I'll see if I can get to it.

@WillBAnders
Copy link
Contributor Author

Tests added here should cover regression already. If you're referring to adding cname-exists as well; I'm all for that and should have time in the next few days.

@WillBAnders
Copy link
Contributor Author

@tschaub Should be good from here. The added test asserts that --cname replaces the existing CNAME value; which matches expectations for how I would expect this to work (and existing behavior).

Hoping for a patch release following this being merged as well so I can close out another separate line of work. Thanks!

@tschaub tschaub merged commit 3312dc4 into tschaub:main Dec 23, 2023
3 checks passed
@tschaub
Copy link
Owner

tschaub commented Dec 23, 2023

Fix included in the 6.1.1 release. Thanks for the contribution, @WillBAnders.

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.

3 participants