-
Notifications
You must be signed in to change notification settings - Fork 79
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
update get deployment query #1365
Conversation
for more information, see https://pre-commit.ci
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.
Left a comment as of now to simplify the logic. Also would love to see some screenshots of the new query working fine with both active Software versions.
houston/deployment.go
Outdated
@@ -551,7 +573,7 @@ func (h ClientImplementation) GetDeployment(deploymentID string) (*Deployment, e | |||
return nil, handleAPIErr(err) | |||
} | |||
|
|||
return &res.Data.GetDeployment, nil | |||
return res.Data.GetDeployment, nil |
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.
nit: Do we expect more than one deployment to be returned by Houston for a query by id? If not, then I would say it might be simpler to check the length of the response and return just the first element, that way function signature would remain the same and we won't require any further changes in the caller functions.
Or if we go by what's currently implemented then in all caller functions instead of directly fetching the first element, we would still need to add check on the length of the array first.
if len(res.Data.GetDeployment) > 0 {
return &res.Data.GetDeployment[0], nil
}
return nil, fmt.Errorf("deployment with id %s not found", deploymentID)
for more information, see https://pre-commit.ci
…stro-cli into update-get-deployment-query
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1365 +/- ##
==========================================
+ Coverage 86.89% 86.91% +0.01%
==========================================
Files 114 114
Lines 13882 13902 +20
==========================================
+ Hits 12063 12083 +20
Misses 1085 1085
Partials 734 734
☔ View full report in Codecov by Sentry. |
…stro-cli into update-get-deployment-query
…stro-cli into update-get-deployment-query
for more information, see https://pre-commit.ci
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.
LGTM
This reverts commit 4fa1191.
Description
The GetDeployment query has been updated with some breaking changes on houston.
This PR changes
id
todeploymentId
🎟 Issue(s)
Related #astronomer/issues/issues/5763
🧪 Functional Testing
unit testing
📸 Screenshots
Only supported on
0.30 : 0.30.8 and upwards
0.32: 0.32.3 and upwards
And 0.33.1 and upwards