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

refactor: DeploymentName to DeploymentKey #1073

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

matt2e
Copy link
Collaborator

@matt2e matt2e commented Mar 13, 2024

This refactors everything to refer to a deployment's key instead of its
name, including:

  • db
  • events
  • console

@matt2e matt2e force-pushed the matt2e/deploymentname-as-struct branch from de5cf77 to 062985c Compare March 13, 2024 21:29
@github-actions github-actions bot changed the title deploymentname as struct feat: deploymentname as struct Mar 13, 2024
@alecthomas alecthomas mentioned this pull request Mar 13, 2024
if name, ok := row.DeploymentName.Get(); ok {
deployment = optional.Some(model.DeploymentName(name))
var zero model.DeploymentName
if row.DeploymentName != zero {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From what I can tell, sqlc won't give us an optional on RunnerRow.DeploymentName because of the coalesce part of the sql statement?

This is also why parsing DeploymentName from "" is returning a DeploymentName without an error, but just a empty deployment name.

Is this all fine in Go or should we be more strict around using optionals vs zero?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wasn't this working before? Why would it change now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed, optionals are maintained like before. And now parsing a deployment name of "" is an error

if name, ok := row.DeploymentName.Get(); ok {
deployment = optional.Some(model.DeploymentName(name))
var zero model.DeploymentName
if row.DeploymentName != zero {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wasn't this working before? Why would it change now?

backend/controller/dal/dal.go Outdated Show resolved Hide resolved
backend/controller/dal/dal.go Outdated Show resolved Hide resolved
@matt2e matt2e requested a review from alecthomas March 14, 2024 03:08
@matt2e matt2e marked this pull request as ready for review March 14, 2024 03:18
@@ -77,6 +81,6 @@ func (d *DeploymentName) Scan(value any) error {
return nil
}

func (d *DeploymentName) Value() (driver.Value, error) {
func (d DeploymentName) Value() (driver.Value, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

sneaking little pointer! :)

@matt2e matt2e force-pushed the matt2e/deploymentname-as-struct branch 3 times, most recently from e9185cf to ab70af2 Compare March 15, 2024 05:12
Separate PR on top of other PR, to separate them out for review

This refactors everything to refer to a deployment's key instead of its
name, including:
- db
- events
- console
@matt2e matt2e force-pushed the matt2e/deploymentname-as-struct branch from ab70af2 to 9fcf325 Compare March 15, 2024 05:19
@matt2e matt2e changed the title feat: deploymentname as struct refactor: DeploymentName to DeploymentKey Mar 15, 2024
@matt2e matt2e merged commit dcfafab into main Mar 15, 2024
9 of 10 checks passed
@matt2e matt2e deleted the matt2e/deploymentname-as-struct branch March 15, 2024 05:20
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.

3 participants