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

Updates project to use cloudflare-go #77

Merged
merged 20 commits into from
Apr 11, 2023

Conversation

KAllan357
Copy link
Contributor

@KAllan357 KAllan357 commented Mar 15, 2023

Hi!

This PR should address a few outstanding issues in the repo. Specifically, #76, #7, and #47.

After executing a pass against a few of the functions inside cloudflare_api.go, I decided that this could go all the way and essentially rewrite the whole file. My high level goal was to keep the API consistent, but adjust the functions so that they used cloudflare-go instead of the net/http package.

Please take a look when you get a chance.

@KAllan357 KAllan357 changed the title updates 'delete tunnel' to use cloudflare-go Updates project to use cloudflare-go Mar 28, 2023
@KAllan357
Copy link
Contributor Author

@adyanth tagging you to see if you'd consider / review this PR.

@adyanth
Copy link
Owner

adyanth commented Mar 28, 2023

Hey @KAllan357 wow, thank you very much for the contribution! I was busy the last week, did not see this. Will review.

Edit: Looks like the CI build does not use the repo secrets for PRs. I'll go through it regardless.

adyanth

This comment was marked as duplicate.

Copy link
Owner

@adyanth adyanth left a comment

Choose a reason for hiding this comment

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

Comments with suggestions

controllers/cloudflare_api.go Outdated Show resolved Hide resolved
controllers/cloudflare_api.go Show resolved Hide resolved
controllers/cloudflare_api.go Outdated Show resolved Hide resolved
KAllan357 and others added 5 commits March 28, 2023 14:16
add tunnelId to error log

Co-authored-by: Adyanth Hosavalike <[email protected]>
return the marshal error for secret creds

Co-authored-by: Adyanth Hosavalike <[email protected]>
remove unused TXT_PREFIX

Co-authored-by: Adyanth Hosavalike <[email protected]>
@KAllan357 KAllan357 requested a review from adyanth April 4, 2023 01:39
@KAllan357
Copy link
Contributor Author

Re-requested review. I'd like to try testing this, but I'm not 100% sure when I can get around to deploying a custom container with my changes.

@adyanth
Copy link
Owner

adyanth commented Apr 4, 2023

The makefile should allow you to build the container. No worries though, let me check it out. Will build and check before merging in. Otherwise, looks good! Thanks again for your contribution!

@KAllan357
Copy link
Contributor Author

FWIW, make build does not seem to work on my machine. Maybe a separate issue to open?

go: creating new go.mod: module tmp
Downloading sigs.k8s.io/controller-tools/cmd/[email protected]
go: added github.com/fatih/color v1.12.0
go: added github.com/go-logr/logr v0.4.0
go: added github.com/gobuffalo/flect v0.2.3
go: added github.com/gogo/protobuf v1.3.2
go: added github.com/google/go-cmp v0.5.6
go: added github.com/google/gofuzz v1.1.0
go: added github.com/inconshreveable/mousetrap v1.0.0
go: added github.com/json-iterator/go v1.1.11
go: added github.com/mattn/go-colorable v0.1.8
go: added github.com/mattn/go-isatty v0.0.12
go: added github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd
go: added github.com/modern-go/reflect2 v1.0.1
go: added github.com/spf13/cobra v1.2.1
go: added github.com/spf13/pflag v1.0.5
go: added golang.org/x/mod v0.4.2
go: added golang.org/x/net v0.0.0-20210520170846-37e1c6afe023
go: added golang.org/x/sys v0.0.0-20210616094352-59db8d763f22
go: added golang.org/x/text v0.3.6
go: added golang.org/x/tools v0.1.5
go: added golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1
go: added gopkg.in/inf.v0 v0.9.1
go: added gopkg.in/yaml.v2 v2.4.0
go: added gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b
go: added k8s.io/api v0.22.2
go: added k8s.io/apiextensions-apiserver v0.22.2
go: added k8s.io/apimachinery v0.22.2
go: added k8s.io/klog/v2 v2.9.0
go: added k8s.io/utils v0.0.0-20210819203725-bdf08cb9a70a
go: added sigs.k8s.io/controller-tools v0.7.0
go: added sigs.k8s.io/structured-merge-diff/v4 v4.1.2
go: added sigs.k8s.io/yaml v1.2.0
/home/kallan/code/cloudflare-operator/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..."
bash: /home/kallan/code/cloudflare-operator/bin/controller-gen: No such file or directory
make: *** [Makefile:80: generate] Error 127

@adyanth
Copy link
Owner

adyanth commented Apr 4, 2023

You need to have controller-gen installed and linked to that folder it mentions for kube builder to work.

Kind of a pain especially in macOS. Brew has it iirc.

https://book.kubebuilder.io/reference/controller-gen.html

Copy link
Owner

@adyanth adyanth left a comment

Choose a reason for hiding this comment

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

Tested it out. This should be all the necessary changes.

controllers/cloudflare_api.go Outdated Show resolved Hide resolved
controllers/cloudflare_api.go Outdated Show resolved Hide resolved
controllers/utils.go Outdated Show resolved Hide resolved
controllers/cloudflare_api.go Outdated Show resolved Hide resolved
controllers/cloudflare_api.go Outdated Show resolved Hide resolved
code fixes from testing the updated code

Co-authored-by: Adyanth Hosavalike <[email protected]>
@KAllan357
Copy link
Contributor Author

Thank you! I just batched all those suggestions up and committed them.

@adyanth adyanth merged commit 9b43440 into adyanth:main Apr 11, 2023
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