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: porkbun provider #217

Merged
merged 21 commits into from
Aug 9, 2021
Merged

add: porkbun provider #217

merged 21 commits into from
Aug 9, 2021

Conversation

fredericrous
Copy link
Collaborator

@fredericrous fredericrous commented Jul 18, 2021

resolves #216

@fredericrous fredericrous requested a review from qdm12 July 18, 2021 20:20
@fredericrous
Copy link
Collaborator Author

lol, I broke your github action @qdm12

@qdm12
Copy link
Owner

qdm12 commented Jul 19, 2021

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!

Copy link
Owner

@qdm12 qdm12 left a 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 😉

docs/porkbun.md Outdated Show resolved Hide resolved
docs/porkbun.md Show resolved Hide resolved
docs/porkbun.md Outdated Show resolved Hide resolved
docs/porkbun.md Outdated Show resolved Hide resolved
internal/settings/providers/porkbun/provider.go Outdated Show resolved Hide resolved
internal/settings/providers/porkbun/provider.go Outdated Show resolved Hide resolved
internal/settings/providers/porkbun/provider.go Outdated Show resolved Hide resolved
internal/settings/providers/porkbun/provider.go Outdated Show resolved Hide resolved
internal/settings/providers/porkbun/provider.go Outdated Show resolved Hide resolved
internal/settings/providers/porkbun/provider.go Outdated Show resolved Hide resolved
fredericrous and others added 9 commits July 19, 2021 09:17
review changes

Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
update apikey to api_key, same for sercretapikey

Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
@fredericrous
Copy link
Collaborator Author

fredericrous commented Jul 19, 2021

Thanks for the review, I'll continue to integrate these feedbacks this evening.
Regarding the container tag. Is this bach? we could do something like echo ::set-output name=version::${TAG##*/} on line 77 there:

echo ::set-output name=version::$TAG

@qdm12
Copy link
Owner

qdm12 commented Jul 19, 2021

Is this bach? we could do something like echo ::set-output name=version::${TAG##*/} on line 77 there:

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

echo ::set-output name=version::$BRANCH
for branches, line 77 is for tags. So feel free to add your bash ##*/ in there 😉 (or not up to you!)

fredericrous and others added 4 commits July 19, 2021 22:59
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
@fredericrous
Copy link
Collaborator Author

I integrated the changes, I think we're good. thanks for having merged the / PR. I used to agree with you but changed my mind mainly because the / is supported in gitkraken (and I think in Git Extension but I never used this one and I'm on mac). Plus the / pattern is ok if we agree on putting one slash and no more

Copy link
Owner

@qdm12 qdm12 left a 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) 😉

docs/porkbun.md Outdated Show resolved Hide resolved
internal/settings/errors/validation.go Outdated Show resolved Hide resolved
internal/settings/errors/validation.go Outdated Show resolved Hide resolved
fredericrous and others added 3 commits August 8, 2021 22:58
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Copy link
Owner

@qdm12 qdm12 left a 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

  1. can you rebase or merge the readme from master so we can merge it please
  2. 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!!

@qdm12 qdm12 merged commit 3ece0c9 into master Aug 9, 2021
@qdm12 qdm12 deleted the provider/porkbun branch August 9, 2021 13:13
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.

Feature request: add porkbun provider
2 participants