-
-
Notifications
You must be signed in to change notification settings - Fork 205
add: porkbun provider #217
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
Conversation
lol, I broke your github action @qdm12 |
😄 yes it's because of the branch name, docker tags don't accept As in it passed the verify stage so that's what matters mostly. I'll review your code now, thanks for the contribution! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice thanks for the contribution! A few comments, hopefully it's not too much 😉
review changes Co-authored-by: Quentin McGaw <[email protected]>
Co-authored-by: Quentin McGaw <[email protected]>
update apikey to api_key, same for sercretapikey Co-authored-by: Quentin McGaw <[email protected]>
Co-authored-by: Quentin McGaw <[email protected]>
Co-authored-by: Quentin McGaw <[email protected]>
Co-authored-by: Quentin McGaw <[email protected]>
Co-authored-by: Quentin McGaw <[email protected]>
Co-authored-by: Quentin McGaw <[email protected]>
Co-authored-by: Quentin McGaw <[email protected]>
Thanks for the review, I'll continue to integrate these feedbacks this evening. ddns-updater/.github/workflows/build.yml Line 77 in bc1a5b2
|
Yes it's bash, but instead try to not use slashes in branches next time 😉 It brings a whole load of problems for git, go etc. I don't recall exactly what it was but it was terrible, another example EDIT: Well it doesn't hurt to fix the CI still so the CI passes here. Note however it's on line ddns-updater/.github/workflows/build.yml Line 83 in bc1a5b2
##*/ in there 😉 (or not up to you!)
|
Co-authored-by: Quentin McGaw <[email protected]>
Co-authored-by: Quentin McGaw <[email protected]>
Co-authored-by: Quentin McGaw <[email protected]>
I integrated the changes, I think we're good. thanks for having merged the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice looking good! Just 3 tiny suggestions below
Also you may have to git rebase master
your branch so that the readme has lfs (and also the CI gets fixed) 😉
Co-authored-by: Quentin McGaw <[email protected]>
Co-authored-by: Quentin McGaw <[email protected]>
Co-authored-by: Quentin McGaw <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice thanks for the changes! A few additional points
- can you rebase or merge the readme from master so we can merge it please
- If you don't mind, can you sneak in the
/
fix you had to the verify job for the publish job as well? If it's too complicated, then we can skip it.
Thanks!!
resolves #216