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

Support for IPv6 clusters #370

Merged
merged 2 commits into from
Mar 14, 2024
Merged

Support for IPv6 clusters #370

merged 2 commits into from
Mar 14, 2024

Conversation

samrocketman
Copy link
Contributor

@samrocketman samrocketman commented Mar 7, 2024

No idea if this works. This is literally my first Go code (my "hello world" but IPv6 attempt).

Closes #369

📑 Description

Detect if a ClusterIP is IPv6 or IPv4. Choose the appropriate string format based on Go documentation.

https://pkg.go.dev/net

A literal IPv6 address in hostport must be enclosed in square brackets, as in "[::1]:80", "[::1%lo0]:80".

I've tested locally that it compiles with make. I've not done any extra testing beyond that. My development environment is Ubuntu 22.04.

✅ Checks

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • All the tests have passed

ℹ Additional Information

I've successfully run make test from my development environment.

$ go version
go version go1.21.6 linux/amd64

@samrocketman samrocketman requested review from a team as code owners March 7, 2024 02:36
Copy link
Contributor

@JuHyung-Son JuHyung-Son 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 contribute.

supporting ipv6 looks good.

and i found that conn does not closed after testing port open.
which is not actually included in the issue.

but can you check that? i guess conn should be closed.

// Test if the port is open
	conn, err := net.DialTimeout("tcp", address, 1*time.Second)
	if err != nil {
		return "", err
	}

pkg/client/client.go Outdated Show resolved Hide resolved
samrocketman and others added 2 commits March 13, 2024 18:01
No idea if this works.  This is literally my first Go code (my "hello world" but IPv6 attempt).

Hopefully fixes k8sgpt-ai#369

closes k8sgpt-ai#369

Signed-off-by: Sam Gleske <sam.mxracer@gmail.com>
* Return err instead of nil
* Defer connection closing

Signed-off-by: JuHyung Son <sonju0427@gmail.com>
Signed-off-by: Sam Gleske <sam.mxracer@gmail.com>

Co-authored-by: JuHyung Son <sonju0427@gmail.com>
Copy link
Contributor

@JuHyung-Son JuHyung-Son left a comment

Choose a reason for hiding this comment

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

looks good to me.
thanks for important contribute

@AlexsJones AlexsJones merged commit 2199d7f into k8sgpt-ai:main Mar 14, 2024
6 checks passed
@samrocketman
Copy link
Contributor Author

This doesn't appear to have been added to the v0.1.1 release notes

@AlexsJones
Copy link
Member

The pull request isn't a conventional commit e.g. feat/chore, so unfortunately the bot didn't pick it up and I didn't notice.

@samrocketman
Copy link
Contributor Author

No worries I was curious is all

@samrocketman
Copy link
Contributor Author

The contributing doc doesn't cover specially formatted commit messages

@samrocketman samrocketman deleted the ipv6 branch March 22, 2024 00:01
@samrocketman
Copy link
Contributor Author

@AlexsJones can you manually update release notes for v0.1.1 GH release to include mention of the new feature? I also suggest updating contributing with what you expect to be specifically formatted messages when contributors open PRs. I'm still not entirely certain what it needed to be.

@samrocketman
Copy link
Contributor Author

@AlexsJones please update the 0.1.1 release notes with my contribution of IPv6 support

1 similar comment
@samrocketman
Copy link
Contributor Author

@AlexsJones please update the 0.1.1 release notes with my contribution of IPv6 support

@samrocketman
Copy link
Contributor Author

samrocketman commented May 2, 2024

@AlexsJones it's not like I was paid for my development time. Attribution is pretty big deal to me and in my opinion I provided you with a major networking feature (IPv6).

It's not great that you're choosing to ignore me and not attribute my work in the release notes. People who follow this project would have seen it so it would have been best on release.

At the very least, updating them after the fact is the next best thing.

Please update the release notes for 0.1.1 release.

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]: Support IPv6 k8s clusters
3 participants