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 EnableDualStack option #707

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Hiroki6
Copy link

@Hiroki6 Hiroki6 commented Aug 5, 2024

@Hiroki6 Hiroki6 force-pushed the s3-dual-stack-option branch 2 times, most recently from bd933e0 to fb8a503 Compare August 5, 2024 14:15
Copy link
Contributor

@agilgur5 agilgur5 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 taking a look and adding this option!

Could you please split the Go module changes into a separate PR? It's not related to this feature and is quite different.
It is also not necessary per my in-line comment as that line does not correspond to the version it compiles with and so may not be accepted/merged at all

s3/s3.go Outdated
@@ -192,7 +193,9 @@ func NewS3Client(ctx context.Context, opts S3ClientOpts) (S3Client, error) {
if opts.Trace {
minioClient.TraceOn(log.StandardLogger().Out)
}

if opts.DisableDualStack {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if opts.DisableDualStack {
if !opts.EnableDualstack {

I would match the existing naming and capitalization of Minio's

Copy link
Author

Choose a reason for hiding this comment

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

I did it intentionally since Minio enables it by default and if the client doesn't specify true in this field explicitly, it always disable it. But, if you think it's fine, I follow your suggestion

go.mod Outdated
@@ -1,6 +1,6 @@
module github.com/argoproj/pkg

go 1.14
go 1.22.5
Copy link
Contributor

@agilgur5 agilgur5 Aug 5, 2024

Choose a reason for hiding this comment

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

Per the Go docs, this is the minimum required Go version. Afaik, this repo does not use any newer Go features and so does not need to raise its version

That is also different from the version this compiles with or is linted with, which is listed in the GHA Workflows, e.g.

go-version: 1.19

The consuming applications also set their own Go versions that compile this package. By definition, they must be at least as high as this go.mod version, which also makes this a breaking change

Copy link
Author

Choose a reason for hiding this comment

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

sure, I reverted it

s3/s3.go Outdated
RoleSessionName string
UseSDKCreds bool
EncryptOpts EncryptOpts
DisableDualStack bool
Copy link
Contributor

@agilgur5 agilgur5 Aug 5, 2024

Choose a reason for hiding this comment

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

I am wondering if we should just inherit options here and parse a JSON blob in Argo Workflows's configuration of extraOpts. As this continues to expand and add to the spec. Allowing arbitrary options may make more sense.

cc @Joibel

Copy link
Author

Choose a reason for hiding this comment

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

you mean like this?

type ExtraOpts struct {
	EnableDualStack bool
}

type S3ClientOpts struct {
        ...
	ExtraOpts       ExtraOpts
}

@Hiroki6 Hiroki6 force-pushed the s3-dual-stack-option branch 3 times, most recently from 9847d93 to 4c3338e Compare August 5, 2024 15:18
@Hiroki6 Hiroki6 requested a review from agilgur5 August 5, 2024 15:20
Hiroki6 and others added 4 commits August 5, 2024 17:23
Signed-off-by: Hiroki6 <[email protected]>
Signed-off-by: Hiroki6 <[email protected]>
Signed-off-by: Hiroki6 <[email protected]>
Signed-off-by: Hiroki6 <[email protected]>
Signed-off-by: Hiroki6 <[email protected]>
Signed-off-by: Hiroki6 <[email protected]>

This comment was marked as resolved.

@Hiroki6 Hiroki6 changed the title bump go version into 1.22.5 & add DisableDualStack option add EnableDualStack option Aug 5, 2024
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