-
Notifications
You must be signed in to change notification settings - Fork 1
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
fix(create-site): update scripts for creating site + publishing #991
base: main
Are you sure you want to change the base?
Conversation
Datadog ReportBranch report: ❌ 3 Failed (0 Known Flaky), 189 Passed, 36 Skipped, 42.4s Total Time ❌ Failed Tests (3)
|
@@ -485,7 +485,6 @@ async function fetchAndWriteSiteData(client: Client) { | |||
const config = { | |||
site: { | |||
...configResult.rows[0].config, | |||
siteName: configResult.rows[0].name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: I recall we did this so that we store the site name as a separate column inside the DB rather than inside the JSON column, as it is used under the sites dashboard. We couldn't remove the siteName
prop inside the JSON blob because then the types would not match.
I think we should still stick to storing the site name in the separate column and have this set to say "Ignore this" (until we have a good solution for the typing mismatch).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: I recall we did this so that we store the site name as a separate column inside the DB rather than inside the JSON column, as it is used under the sites dashboard. We couldn't remove the siteName prop inside the JSON blob because then the types would not match.
can clarify this? not sure what the issue here is apart from the types - if it's just about the name that presents on the dashboard, we can also chagne it to read from site.config.siteName
, which i thought is better because that's a "user level view" whereas the db one (site.name
) is not tied under site.config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah it would be perfect if we can read directly from site.config.siteName
but not sure if there is any performance impact reading from JSON blobs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok then i'll just make the change to read directly from site.config.siteName
HAHA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, see video here. re: performance impact - it's close to 0 because we already read it at present lol
NOTE: do not merge till release as this will go into effect IMMEDIATELY
Problem
https://opengovproducts.slack.com/archives/C06R4DX966P/p1736928912803439
siteName
- which in turns impact the footercreateSite.ts
has a defaultsiteName
set, even though the script already gives asiteName
Solution
config
object already hassiteName
siteName
forcreateSite
Tests
createSite.ts
siteName
and not the default of MTIsite.name
andsite.config.siteName
differsite.config.siteName