search

xanzy / go-gitlab

GitLab Go SDK
Apache License 2.0
2.33k stars 924 forks source link

GitLab 17.0: Fix an issue where Settings errors when ContainerRegistry is not enabled #1938

Closed PatrickRice-KSC closed 1 month ago

PatrickRice-KSC commented 2 months ago

In the 17.0-nightly build, the ContainerRegistryImportCreatedBefore attribute now returns "" when the container registry isn't enabled. This causes a parsing error because an empty string isn't parseable as a date in Go. As a result, GetSettings fails on every instance without a container registry enabled.

This PR implements custom parsing logic for the Settings struct that discards that value if it's set to an empty string to ensure that the Settings can continue to parse properly.

You can see this being detected in the Terraform Provider Nightly tests here: https://gitlab.com/gitlab-org/terraform-provider-gitlab/-/jobs/6827815421#L1850

PatrickRice-KSC commented 2 months ago

@svanharmelen - This should be ready for review!

To be honest, I don't love that we have to implement this here because it seems almost like a data type issue in the GitLab API. I'll chat with GitLab about it as well, but I think it's safe and relatively innocuous to handle it here as well.

I also debated using a string replace to remove the value instead of using the unmarshal -> remarshal approach, since the double marshal is "expensive" (relatively speaking) from a CPU perspective. I landed on doing the two step marshal instead because I think it's easier to grok from a readability perspective, and I think it's overall safer from a deterministic behavior perspective. Let me know if you think the other approach would be preferable though.

If you agree and merge, would you mind cutting a new version? That way I can see if the nightly builds get fixed on the TF provider :)

PatrickRice-KSC commented 2 months ago

Update from discussions with the GitLab team - this field is only intended to be used on gitlab.com, so it's removal in 17.0 is intentional. We could either remove the field from the struct completely, or handle parsing the blank value (like this PR will do): https://gitlab.com/gitlab-org/gitlab/-/merge_requests/149168

I'd lean towards handling the blank value in case someone is using those fields (and marking them as deprecated so people can remove them over the next couple of months), but I'm interested in your thoughts!