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

feat(domain-v1): support v1 functionality #917

Merged
merged 12 commits into from
Jan 31, 2025
Merged

Conversation

Integralist
Copy link
Collaborator

@Integralist Integralist commented Jan 27, 2025

No description provided.

@Integralist Integralist changed the title Integralist/domains v1 feat(domains): support v1 functionality Jan 27, 2025
@Integralist Integralist changed the title feat(domains): support v1 functionality feat(domain-v1): support v1 functionality Jan 27, 2025
Copy link
Member

@tundal45 tundal45 left a comment

Choose a reason for hiding this comment

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

One comment and one non-blocking question. Looks good!

@Integralist Integralist force-pushed the integralist/domains-v1 branch from 0eec7fa to a968b3c Compare January 30, 2025 20:17
@Integralist Integralist force-pushed the integralist/domains-v1 branch from a968b3c to 95cd7b9 Compare January 30, 2025 20:21
@Integralist
Copy link
Collaborator Author

Integralist commented Jan 30, 2025

@philippschulte might need some help with this PR as I honestly don't know what's going on with this PR and the failing tests.

For example, make vet passes for me locally but in the CI we're getting...

golang.org/x/mod/internal/lazyregexp: cannot compile Go 1.22 code
golang.org/x/mod/semver: cannot compile Go 1.22 code

Vet found suspicious constructs. Please check the reported constructs
and fix them if necessary before submitting the code for review.
make: *** [Makefile:[7](https://github.com/fastly/terraform-provider-fastly/actions/runs/13060722351/job/36442779461?pr=917#step:6:8)0: vet] Error 1

Another example, make test passes for me locally but in the CI we're getting...

go  test $(go  list ./... |grep -v 'vendor') || exit 1
?   	github.com/fastly/terraform-provider-fastly	[no test files]
golang.org/x/mod/internal/lazyregexp: cannot compile Go 1.22 code
golang.org/x/mod/semver: cannot compile Go 1.22 code
?   	github.com/fastly/terraform-provider-fastly/fastly/hashcode	[no test files]
?   	github.com/fastly/terraform-provider-fastly/version	[no test files]
FAIL	github.com/fastly/terraform-provider-fastly/fastly [build failed]
FAIL
make: *** [Makefile:41: test] Error 1

@Integralist
Copy link
Collaborator Author

@philippschulte I'm not entirely sure but it looks like the issue with this TF PR failing...

golang.org/x/mod/internal/lazyregexp: cannot compile Go 1.22 code
golang.org/x/mod/semver: cannot compile Go 1.22 code

...is stemming from a recent update to go-fastly (https://github.com/fastly/go-fastly/pull/580/files).

In that go-fastly PR there appears to be an indirect dependency bump:

- golang.org/x/mod v0.14.0 // indirect
+ golang.org/x/mod v0.22.0 // indirect

The 0.22.0 version of x/mod (link) contains:

go 1.22.0

We have to go back to version 0.20.0 for it to be compatible:

go 1.18

This means, when we vendor the dependencies in TF we see in the go.sum:

# golang.org/x/mod v0.22.0
## explicit; go 1.22
golang.org/x/mod/internal/lazyregexp
golang.org/x/mod/modfile
golang.org/x/mod/module
golang.org/x/mod/semver

I think the latest go-fastly release is going to force us to have to bump the go toolchain version for TF up to 1.22.0.

We can't revert that change as we've published a release (9.13.0) since then.

I don't think that should be an issue per-se, as the TF provider is a compiled binary (much like the Fastly CLI) and so it isn't embedded into user code like go-fastly is.

Thoughts?

cc @kpfleming

@Integralist Integralist force-pushed the integralist/domains-v1 branch from 019ec2c to 76b111c Compare January 31, 2025 11:29
@Integralist
Copy link
Collaborator Author

OK. I've gone ahead and updated this project to use Go 1.22.
This also required an update to goreleaser.

See commit for details: 76b111c

After making the changes in 76b111c I ran the goreleaser target to validate...

$ make goreleaser GORELEASER_ARGS="--skip=validate --clean"

go  install github.com/goreleaser/goreleaser/v2@latest
go: github.com/goreleaser/goreleaser/v2@v2.6.1 requires go >= 1.23.4; switching to go1.23.5
  • skipping validate...
  • cleaning distribution directory
  • loading environment variables
  • getting and validating git state
    • git state                                      commit=14ad9f4844aece0cd277340325b1d9bedc8a39fb branch=integralist/domains-v1 current_tag=v5.15.0 previous_tag=v5.14.0 dirty=true
    • pipe skipped                                   reason=validation is disabled
  • parsing tag
  • setting defaults
  • running before hooks
    • running                                        hook=go mod tidy
  • ensuring distribution directory
  • setting up metadata
  • writing release metadata
  • loading go mod information
  • build prerequisites
  • building binaries
    • building                                       binary=dist/terraform-provider-fastly_freebsd_386_sse2/terraform-provider-fastly_v5.15.0
    • building                                       binary=dist/terraform-provider-fastly_windows_arm64_v8.0/terraform-provider-fastly_v5.15.0.exe
    • building                                       binary=dist/terraform-provider-fastly_windows_arm_6/terraform-provider-fastly_v5.15.0.exe
    • building                                       binary=dist/terraform-provider-fastly_windows_amd64_v1/terraform-provider-fastly_v5.15.0.exe
    • building                                       binary=dist/terraform-provider-fastly_freebsd_arm64_v8.0/terraform-provider-fastly_v5.15.0
    • building                                       binary=dist/terraform-provider-fastly_freebsd_arm_6/terraform-provider-fastly_v5.15.0
    • building                                       binary=dist/terraform-provider-fastly_windows_386_sse2/terraform-provider-fastly_v5.15.0.exe
    • building                                       binary=dist/terraform-provider-fastly_freebsd_amd64_v1/terraform-provider-fastly_v5.15.0
    • building                                       binary=dist/terraform-provider-fastly_linux_amd64_v1/terraform-provider-fastly_v5.15.0
    • building                                       binary=dist/terraform-provider-fastly_linux_386_sse2/terraform-provider-fastly_v5.15.0
    • building                                       binary=dist/terraform-provider-fastly_linux_arm_6/terraform-provider-fastly_v5.15.0
    • building                                       binary=dist/terraform-provider-fastly_linux_arm64_v8.0/terraform-provider-fastly_v5.15.0
    • building                                       binary=dist/terraform-provider-fastly_darwin_amd64_v1/terraform-provider-fastly_v5.15.0
    • building                                       binary=dist/terraform-provider-fastly_darwin_arm64_v8.0/terraform-provider-fastly_v5.15.0
    • took: 1m31s
  • writing artifacts metadata
  • build succeeded after 1m31s
  • thanks for using GoReleaser!

Copy link
Member

@philippschulte philippschulte left a comment

Choose a reason for hiding this comment

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

I think the latest go-fastly release is going to force us to have to bump the go toolchain version for TF up to 1.22.0.

We can't revert that change as we've published a release (9.13.0) since then.

I don't think that should be an issue per-se, as the TF provider is a compiled binary (much like the Fastly CLI) and so it isn't embedded into user code like go-fastly is.

I agree and to my knowledge Kevin wanted to bump Go to 1.22 (#899) anyway.

Please just update the Requirements and Developing the Provider sections in the README.md.

I am planing to cut a new release today. Thank you!

@Integralist
Copy link
Collaborator Author

Thanks @philippschulte I've pushed a commit to update the README now 👍🏻

Copy link
Member

@philippschulte philippschulte left a comment

Choose a reason for hiding this comment

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

Thank you!

@Integralist Integralist merged commit 439ac3d into main Jan 31, 2025
11 checks passed
@Integralist Integralist deleted the integralist/domains-v1 branch January 31, 2025 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants