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: do not use only hardcoded "go" #97

Merged
merged 3 commits into from
Apr 25, 2022
Merged

fix: do not use only hardcoded "go" #97

merged 3 commits into from
Apr 25, 2022

Conversation

kmpm
Copy link
Contributor

@kmpm kmpm commented Apr 14, 2022

Quick and dirty fix for #96

If you set a environment variable XCADDY_SDK to whatever you want to use instead of "go" then xcaddy will use it.

example

XCADDY_SDK=go1.17.9 xcaddy build

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Thanks for the patch!

I like how this works, I would just ask if we can rename the env variable to something less obscure (I know what SDK is, but it's not a term we hear much in the Go ecosystem).

Also, can we please move the new function into an existing file? It seems odd to make a whole new file for a 5 line function. More of the file is boilerplate than code...

Otherwise, LGTM

sdk.go Outdated Show resolved Hide resolved
Copy link
Member

@mohammed90 mohammed90 left a comment

Choose a reason for hiding this comment

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

Only one nit-pick: unexport the GetGo function by changing it to getGo

@mholt
Copy link
Member

mholt commented Apr 18, 2022

@mohammed90 I'd prefer that too, but it's used outside the package...

@mohammed90
Copy link
Member

@mohammed90 I'd prefer that too, but it's used outside the package...

We can use the /internal package pattern to hide it from the outside world but be exported for internal (within xcaddy) usage.

@kmpm
Copy link
Contributor Author

kmpm commented Apr 19, 2022

Should I create a /internal file with all the extra boilerplate that really isn't necessary or move the logic in to an existing file and keep the exported name?

@mholt
Copy link
Member

mholt commented Apr 20, 2022

@kmpm I think all we need to do is move GetGo into a new file in an /internal/ folder and then use it within xcaddy. It can still be exported since it won't be usable outside of xcaddy.

@kmpm
Copy link
Contributor Author

kmpm commented Apr 20, 2022

@kmpm I think all we need to do is move GetGo into a new file in an /internal/ folder and then use it within xcaddy. It can still be exported since it won't be usable outside of xcaddy.

@mholt Yes... but it's needed in cmd/main.go which can not reach anything in xcaddy.internal, or how do I get around that?

@mholt
Copy link
Member

mholt commented Apr 20, 2022

Oh... main() can't access internal within the same Go module? That's surprising.

I'm not sure. I will have to read more about it.

@mohammed90
Copy link
Member

Oh... main() can't access internal within the same Go module? That's surprising.

I'm not sure. I will have to read more about it.

You can. I've done it before. Let me try this coming Friday.

@mohammed90
Copy link
Member

@kmpm, there's a little checkbox on the side labelled "Allow edits by maintainers". Can you verify it's checked? I've moved the func to an internal package locally and need to just push the small change to the same PR.

@kmpm
Copy link
Contributor Author

kmpm commented Apr 22, 2022

@mohammed90 , the box is checked. I'm looking forward to see how you solved it.

P.S. Working in a different timezone than the maintainers is no fun and I do apologize.

@mohammed90
Copy link
Member

P.S. Working in a different timezone than the maintainers is no fun and I do apologize.

I'm only 1 hour ahead of your timezone 😁 but I only work on open source projects in the evenings

Copy link
Contributor Author

@kmpm kmpm left a comment

Choose a reason for hiding this comment

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

I like it

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

LGTM now, thank you both!

@mholt mholt merged commit 7abc7f5 into caddyserver:master Apr 25, 2022
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.

None yet

4 participants