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

Upgrade dependencies #853

Merged
merged 1 commit into from Sep 9, 2022
Merged

Upgrade dependencies #853

merged 1 commit into from Sep 9, 2022

Conversation

amuraru
Copy link
Contributor

@amuraru amuraru commented Aug 19, 2022

Q A
Bug fix? no
New feature? no
API breaks? no
Deprecations? no
License Apache 2.0

What's in this PR?

  • Upgrade to go 1.19
  • Upgrade all dependencies

Why?

Keeping dependencies up to date

Checklist

  • Implementation tested

@amuraru amuraru requested a review from a team as a code owner August 19, 2022 16:34
@amuraru amuraru force-pushed the deps branch 4 times, most recently from c9b8048 to b458adb Compare August 22, 2022 08:42
ctrlaltluc
ctrlaltluc previously approved these changes Aug 23, 2022
panyuenlau
panyuenlau previously approved these changes Aug 23, 2022
Copy link
Member

@panyuenlau panyuenlau 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 upgrading to Go 1.19 😉

Kuvesz
Kuvesz previously approved these changes Aug 23, 2022
hi-im-aren
hi-im-aren previously approved these changes Aug 23, 2022
pregnor
pregnor previously approved these changes Aug 23, 2022
Copy link
Member

@pregnor pregnor left a comment

Choose a reason for hiding this comment

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

LGTM, thank you so much for the contribution.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
pregnor
pregnor previously approved these changes Sep 5, 2022
Copy link
Member

@pregnor pregnor left a comment

Choose a reason for hiding this comment

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

LGTM, if it behaved well on the tests, I'm fine merging this.

@@ -27,7 +27,8 @@ import (
)

var log = logf.Log.WithName("kafka_util")
var apiVersion = sarama.V2_6_0_0
var apiVersion = sarama.MaxVersion
Copy link
Member

Choose a reason for hiding this comment

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

According to https://github.com/Shopify/sarama/blob/v1.36.0/config.go#L455

// The version of Kafka that Sarama will assume it is running against.
	// Defaults to the oldest supported stable version. Since Kafka provides
	// backwards-compatibility, setting it to a version older than you have
	// will not break anything, although it may prevent you from using the
	// latest features. Setting it to a version greater than you are actually
	// running may lead to random breakage.
	Version KafkaVersion

This means that using sarama.MaxVersion api version which points to V3_2_0_0 may cause to random issues in koperator when interacting with older Kafka versions (e.g. 2.6).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah in this case I support leaving it on the older version for backward compatibility reasons.
If this is a blocker for you Adrian, let us know and we will think about what else we could do.

My apologies for missing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kafka clients since 1.x are also backward compatible with older versions of brokers - in the sense that they negotiate the latest API version they can talk during the initial handshake. I believe that comment is a very old one (6 yrs back IBM/sarama@5e8b6ff) when that was not the case.

Either way - I don't have a strong opinion and I can revert back this.
The intent was to ensure we always use the latest API version sarama client can talk and not having to worry about in the feature about bumping this in needed.

@pregnor pregnor dismissed their stale review September 7, 2022 15:11

See Seba's comment and my subcomment.

pkg/kafkaclient/client.go Outdated Show resolved Hide resolved
- Upgrade to go 1.19
- Upgrade all dependencies

Signed-off-by: Adi Muraru <amuraru@adobe.com>
.golangci.yml Show resolved Hide resolved
@pregnor pregnor requested a review from stoader September 8, 2022 10:52
Copy link
Contributor

@Kuvesz Kuvesz 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! Thanks for the upgrade :)

@pregnor
Copy link
Member

pregnor commented Sep 9, 2022

I'm planning to have this merged end of business day unless anyone objects.

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

7 participants