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

exoscale: update Egoscale to v3.1.1 #2256

Merged
merged 5 commits into from
Sep 10, 2024
Merged

exoscale: update Egoscale to v3.1.1 #2256

merged 5 commits into from
Sep 10, 2024

Conversation

mlec1
Copy link
Contributor

@mlec1 mlec1 commented Aug 31, 2024

Update Egoscale library for the Exoscale DNS provider to v3.

v1 and v2 are deprecated: https://github.com/exoscale/egoscale?tab=readme-ov-file#deprecated-use-v3

exoscale/egoscale@v0.102.3...v3.1.1

@ldez ldez self-requested a review August 31, 2024 11:03
@ldez ldez changed the title feat(exoscale): Update Egoscale to v3 exoscale: update Egoscale to v3.1.1 Aug 31, 2024
@ldez ldez added this to the v4.19 milestone Aug 31, 2024
Copy link

@pierre-emmanuelJ pierre-emmanuelJ left a comment

Choose a reason for hiding this comment

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

👍 LGTM

Thanks, @mlec1, for the contribution! And thanks @ldez for the review :)

@@ -84,12 +81,8 @@ func NewDNSProviderConfig(config *Config) (*DNSProvider, error) {
return nil, errors.New("exoscale: credentials missing")
}

egoscale.UserAgent = "go-acme/lego " + egoscale.UserAgent

Choose a reason for hiding this comment

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

Any reason for dropping it?

Copy link
Member

@ldez ldez Sep 2, 2024

Choose a reason for hiding this comment

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

Because it's a global variable, I don't want to produce side effects: if someone uses lego and exocale as a lib, modifying the global variable will be a problem.

If lego defines the global and an app uses exocale, what will be the user agent at the end?
It's not possible to know because it will depend on when and where the user agent is set:

  • If lego is the last to set the variable then all the calls will have the lego user agent.
  • If lego is the first to set the variable then all the calls will have the app user agent.
  • If the app and/or lego defined the user agent through another global variable, it will be "random".

Another problem: with the global variable, if the user defines the user agent then information about the client (versions, etc.) will be discarded, expect if the user appends the previous user agent manually.

Generally speaking, global variables should be avoided for a lib.

Copy link
Member

Choose a reason for hiding this comment

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

IMHO, the User-Agent should be an option of the Client like the other ClientOpt instead of a global variable.

Copy link
Member

@ldez ldez Sep 2, 2024

Choose a reason for hiding this comment

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

I was hoping something like that: exoscale/egoscale@master...ldez:egoscale:feat/user-agent

I will not open a PR because it's a breaking change, except if you want it.

EDIT: I created a now-breaking version.

Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM

@ldez ldez merged commit 65cd007 into go-acme:master Sep 10, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants