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

[CMSP-217] Accept siteID key as either int or string #1

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

lizard-boy
Copy link

As part of implementing pantheon-systems/terminus-yml-validator-plugin#1, I discovered it is difficult, if not impossible, for PHP to differentiate between siteID keys being an int, or a string that could be cast to an int.

Rather than jumping through a lot of hoops on the PHP side, I opted to make the go validator a bit more flexible.

TODO:

  • assert how the search-replace script behaves if the key is an int in one case and a string in the other, and adapt accordingly (this can be done as part of upgrading sites-yml-validator in the job worker and does not need to block PR).

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This pull request modifies the sites-yml-validator to accept site IDs as either integers or strings in YAML configurations, addressing difficulties in PHP differentiating between integer and string keys.

  • Modified pkg/model/sites.go to use string keys for DomainMapByEnvironment type
  • Updated pkg/validator/sites.go with new isValidSiteID function to validate site IDs
  • Added new test cases in pkg/validator/sites_test.go for string-based site IDs
  • Introduced new fixture files valid_both_string_and_int_key.yml and valid_string_as_key.yml to test mixed and string-only site ID representations

5 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings

Comment on lines +5 to +7
1: dev-srtest.pantheonsite.io
"2": about.dev-srtest.pantheonsite.io
"3": employee-resources.dev-srtest.pantheonsite.io
Copy link

Choose a reason for hiding this comment

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

style: Inconsistent key formatting for site IDs. Consider using quotes for all keys for consistency.

Comment on lines +5 to +7
"30": dev-srtest.pantheonsite.io
"2": about.dev-srtest.pantheonsite.io
"3": employee-resources.dev-srtest.pantheonsite.io
Copy link

Choose a reason for hiding this comment

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

style: Ensure consistent quoting style for all string keys

Comment on lines +96 to +98
if s[0:1] == "0" {
return false
}
Copy link

Choose a reason for hiding this comment

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

style: This check might reject valid site IDs like '0123'. Consider using a regex to check for leading zeros instead

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.

2 participants