appconfig: replace deprecated AWS API

[thnk2wn]

For issue #312

[knadh]

Thanks @thnk2wn. Are these changes backwards compatible within the provider? If not, we should break it to provider to /v3.

Also, please check the failing test. I don't think go.work should have those changes to ensure backwards compatibility for the whole repo: https://github.com/knadh/koanf/actions/runs/10816433375/job/30067553008?pr=313

[thnk2wn]

Thanks @thnk2wn. Are these changes backwards compatible within the provider? If not, we should break it to provider to /v3.

Also, please check the failing test. I don't think go.work should have those changes to ensure backwards compatibility for the whole repo: https://github.com/knadh/koanf/actions/runs/10816433375/job/30067553008?pr=313

@knadh Yeah mentioned in the issue that it wasn't backwards compatible. It's only removing ClientID which we could keep and mark deprecated but then it wouldn't be used. If that's preferred I can do that but otherwise will plan on bumping to v3.

Regarding go.work it shouldn't be in the repo to begin with IMO:

• GitHub's default gitignore includes it: GitHub's default gitignore includes the go.work file.
• Official Go repositories don't check it in: Official Go repositories, like x/tools, don't check in go.work files.
• It's used for local development: Go.work files are used for local development, and it's recommended to not commit them.

I think it's a later version of Go on my system impacting the the tests. I can look at adjusting Go.work but I think there's a larger issue/question on why go.work is committed to begin with.

[thnk2wn]

@knadh This is done in 9531972. Note however that along with upgrading AWS SDK for Go to the latest for appconfig, that now requires a minimum version of Go 1.21. If there's a requirement to support that module with older Go runtimes we'd have to undo the AWS SDK upgrade (or find right earlier version) but don't really recommend that.

I don't think I have the ability to re-run the workflow. We might consider a workflow dispatch to the github action so it could be run manually

[rhnvrm]

Would adding a build constraint make sense? IIRC, we had done something similar in #89

[thnk2wn]

Would adding a build constraint make sense? IIRC, we had done something similar in #89

@rhnvrm Happy to add //go:build go1.21 within providers/appconfig but it won't solve the problem with go.work:

go: module providers/appconfig listed in go.work file requires go >= 1.21, but go.work lists go 1.18; to update it: go work use

Again to me go.work shouldn't be in the repo to begin with but if it's left there, AFAIK there's no way to have it support different Go versions across the modules

[rhnvrm]

Again to me go.work shouldn't be in the repo to begin with but if it's left there, AFAIK there's no way to have it support different Go versions across the modules

Just revisited why we had added this. We had realized that we were not running submodule tests and after going through this thread (and comment) we fixed broken tests and started to run tests via #276

golang/go#59480 (comment)

It streamlines the local testing and the CI step. Allowing contributors to run a simple go test command.

But yes, I agree, after going through more recent threads such as golang/go#53502 and golang/go#59480 I think it might not be worth it. And as you mentioned, with the versioning issue, it will not work at all.

In this WIP branch, I have removed the go.work, and changed it to a sample file, which can be used for local testing and development. Also, added a section in the README for documenting this. Since the go.work is un-checked-in, changed the CI test definition as well.

master...rhnvrm:koanf:fix-remove-gowork

@thnk2wn @knadh please share your thoughts on this.

[thnk2wn]

@rhnvrm @knadh Thanks, that context is helpful.

I like the idea of that go.work.sample as it solves the problem with go.work while still making it easy to create and use a workspace with all the modules.

I think this approach would also work for iterating and running all the tests but can't say it's any better or worse than workflow steps:

find . -name go.mod -execdir go test -v ./... ;

Probably makes sense to keep it dynamic although there's a chance it might test more or less than desired depending on future directory changes and conventions. Guessing it outweighs maintenance of being explicit with each path.

[thnk2wn]

@rhnvrm @knadh Thanks, that context is helpful.

I like the idea of that go.work.sample as it solves the problem with go.work while still making it easy to create and use a workspace with all the modules.

I think this approach would also work for iterating and running all the tests but can't say it's any better or worse than workflow steps:

find . -name go.mod -execdir go test -v ./... ;

Probably makes sense to keep it dynamic although there's a chance it might test more or less than desired depending on future directory changes and conventions. Guessing it outweighs maintenance of being explicit with each path.

@rhnvrm Anything we can do to move this along? If #317 is merged it could also help resolve this, with or without the removal of go.work.

[knadh]

@rhnvrm ?