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

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

Merged
merged 2 commits into from May 20, 2024

Conversation

PatrickRice-KSC
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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!

@PatrickRice-KSC PatrickRice-KSC changed the title GitLab Nightly: Fix an issue where Settings errors when ContainerRegistry is not enabled GitLab 17.0: Fix an issue where Settings errors when ContainerRegistry is not enabled May 19, 2024
Copy link
Member

@svanharmelen svanharmelen left a comment

Choose a reason for hiding this comment

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

I simplified the solution just a little and also removed the test to check if unmarshalling an empty string would fail.

Settings is an object so it would be a JSON error if you try to parse "" into a Settings object (should always be null, which I did test and works as expected).

@svanharmelen svanharmelen merged commit c93087f into xanzy:main May 20, 2024
3 checks passed
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

2 participants